Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to linear #70

Closed
wants to merge 10 commits into from
Closed

Port to linear #70

wants to merge 10 commits into from

Conversation

cchalmers
Copy link
Member

Added functions fromOrthogonal and fromSymmetric, eye and scaleFromTransform to Diagrams.Core.Transform.

lens >= 4.0 && < 4.4,
linear,
adjunctions,
distributive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some version bounds for these new dependencies.

@cchalmers
Copy link
Member Author

Thanks for all the comments. Most of the fixes where simple but my function comments might need looking over.

I think the old HasLinearMap (V t) => Transformable t was there because of vector-space's constraints on the linear map type from Data.LinearMap. The new HasLinearMap is just a bunch of instances (Representable, Traversable etc.) used to make things like identity matrices. Most transforms don't make use of it. In fact, looking through the code again HasLinearMap could probably be removed in a lot of the other code.

@bergey
Copy link
Member

bergey commented Sep 7, 2014

Put another way, HasLinearMap in master ensures that the transformation can be memoized, via HasTrie. The Transformation in this branch is not memoized.

@bergey
Copy link
Member

bergey commented Sep 7, 2014

linear does not provide a class like vector-space's AdditiveGroup, representing types that have addition but not multiplication. Additive is a slightly weaker Applicative -- unlike Applicative, Additive can be defined for sparse vectors. Hence the bogus Applicative instance for Measure, to preserve the ^+^ operator.

I think the right thing to do is to drop the bogus Applicative and Additive instances. I don't see that they are needed anywhere in -lib. We export the constructors of Measure, so PlusM is available. I doubt it's needed often. If people really want an operator, we can define Semigroup and Monoid, but it's fairly arbitrary to make <> PlusM rather than MinM or MaxM.

@byorgey
Copy link
Member

byorgey commented Sep 8, 2014

I agree, having bogus instances around just to support some syntax sugar doesn't sound like a good bargain.

@jeffreyrosenbluth
Copy link
Member

I think it is import to make sure we remove any superfluous uses of HasLineraMap, as @cchalmers mentions above. If it turns out that there are only a few uses then we should probably drop it and spell out the individual constraints.

@cchalmers
Copy link
Member Author

I don't like the bogus instance but I do want Measure to be Additive. I've come up with a sketch of a different representation here: http://lpaste.net/110692. Could alternatively get derived instances using different newtype:

-- getMeasure :: (globalScale, normalizedScale) -> V2 output local
newtype Measure' n a = Measure ((n, n) -> a)
  deriving (Typeable, Functor, Additive)

type Measure n = Measure' n (V2 n)

It looks like it can do everything the old Measure could while satisfying Additive laws. What do you think?

@bergey
Copy link
Member

bergey commented Sep 8, 2014

@cchalmers Can you explain how you would use the Additive instance?

I think your proposed representation is missing the conversion from (pre-scaled) Local to Output, and it should probably read fromOutput = fromMeasure 1 1. In general, though, it seems feasible.

I'm hoping @jeffreyrosenbluth has thoughts on this design, since I don't use the power of Measure myself.

@jeffreyrosenbluth
Copy link
Member

@cchalmers Honestly I really don't see the benefit of having Measure be this complex. I'm more in the camp of @Mathnerd314 and simplifying Measure. Please take a look at the relevant parts of this discussion #65 (comment) and let me know what you think. @byorgey may have a strong feeling here, I'd like to know what he and @bergey think.

@cchalmers
Copy link
Member Author

@bergey I guess it comes down to future uses of Measure like diagrams/diagrams-lib#208. I agree there isn't much use in adding measures for things like LineWidth.

@jeffreyrosenbluth I don't feel it's that "complex". I've made some changes: http://lpaste.net/110692 maybe this is better? There's not much difference to the user and less code.

For me this representation matches my intuition for what a measure is -- a number that depends of scales that it doesn't have yet. And it's a lot more versatile (you can even write things like Measured n (Diagram b v n)).

For now I'll ditch Additive along with other unused stuff and make a PR if I can find any use for my new Measure.

@jeffreyrosenbluth
Copy link
Member

Yes, that's starting to look good to me. I don't know why I thought the
other looked complex, I probably looked at it too quickly. Why don't you
put it in the fork and see what other changes would be required.

On Mon, Sep 8, 2014 at 4:56 PM, Chris [email protected] wrote:

@bergey https://github.com/bergey I guess it comes down to future uses
of Measure like diagrams/diagrams-lib#208
diagrams/diagrams-lib#208. I agree there
isn't much use in adding measures for things like LineWidth.

@jeffreyrosenbluth https://github.com/jeffreyrosenbluth I don't feel
it's that "complex". I've made some changes: http://lpaste.net/110692
maybe this is better? There's not much difference to the user and it's
certainly a lot less code.

For me this representation matches my intuition for what a measure is -- a
number that depends of scales that it doesn't have yet. And it's a lot more
versatile (you can even write things like Measured n (Diagram b v n)).

For now I'll ditch Additive along with other unused stuff and make a PR
if I can find any use for my new Measure.


Reply to this email directly or view it on GitHub
#70 (comment).

@bergey
Copy link
Member

bergey commented Sep 9, 2014

I think delaying this until there are more examples of how it could be used is a good plan. @cchalmers, thank you for being flexible. In particular, I think the Measure code has been a substantial source of bugs in the past, so I expect more testing would be needed than for most similar-sized changes. I'd rather not do that just for the ^+^ synonym.

I agree with @jeffreyrosenbluth that the second version is more elegant, and has a more intuitive Applicative instance.

@cchalmers cchalmers closed this Sep 14, 2014
@cchalmers cchalmers mentioned this pull request Sep 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants