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

update for linear #40

Merged
merged 13 commits into from
Oct 10, 2014
Merged

update for linear #40

merged 13 commits into from
Oct 10, 2014

Conversation

bergey
Copy link
Member

@bergey bergey commented Sep 13, 2014

Accompanies diagrams/diagrams-lib#212

This is nearly done; it remains to decide what to do about force-layout, and the minor name collision of size. I think the choices with force-layout are port it to linear (it has no revdeps but diagrams-contrib) or create vector-space class instances for V2 and other types, in diagrams-contrib.

replace R2 with R2 n, suitable constraints on n
dmod :: RealFrac a => a -> a -> a
-- | Interpolate linearly between two values. The first argument is
-- the parameter. A parameter of @0@ results in the second argument;
-- with a parameter of @1@, @lerp'@ returns its third argument.
Copy link
Member

Choose a reason for hiding this comment

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

I think you've got these the wrong way round :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that!

@cchalmers
Copy link
Member

I was actually working on this too :). nvm, it's nice to compare. I've also converted force-layout and diagrams-svg as well as some others but I don't have permission to push.

@bergey
Copy link
Member Author

bergey commented Sep 14, 2014

I should have checked with you before I started. Sorry. I'd like to work on a Backend or two. What have you not started on?

I just gave you push rights to more repos, including -svg and force-layout. Let me know if you need access to something I missed. And when you push to new branches, please do call them linear so that travis finds the right branches of -core and -master.

@jeffreyrosenbluth
Copy link
Member

I'd like to linearize at least one backend as well. Let me know whats left.

@cchalmers
Copy link
Member

Great, thanks. So far I've worked on diagrams-backend-tests and active as well. I'd also like to do -rasterific but that's it for backends for now.

@jeffreyrosenbluth
Copy link
Member

@bergey why dont you take -cairo or -canvas, your choice and i'll take the other.

@jeffreyrosenbluth
Copy link
Member

@cchalmers I gave you push rights to diagrams-rasterific

@bergey
Copy link
Member Author

bergey commented Sep 14, 2014

@jeffreyrosenbluth I'll take -cairo. Please take -canvas.

@jeffreyrosenbluth
Copy link
Member

@bergey sounds good. will do

bergey and others added 7 commits September 14, 2014 22:19
Follows the argument order in vector-space.
More type class constraints in Logo
In liftF & liftF2, `a` and `n` are distinct type variables,
since `f` must operate both on `n ` and `Complex n`
@@ -101,8 +99,8 @@ data Q236 = Q236 Rational Rational Rational Rational
deriving (Eq, Ord, Show, Read)
Copy link
Member

Choose a reason for hiding this comment

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

This Ord instance is wrong? It's used for Grid but I don't fully understand it. A proper Ord instance could be made by comparing Doubles:

instance Ord Q236 where
  a <= b = toDouble a <= toDouble b

Same trick could be done for signum.

Copy link
Member Author

Choose a reason for hiding this comment

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

That Ord instance has been there longer than I've been involved, so I hadn't really thought about it. I agree, it doesn't do what I'd expect (though it's adequate for Map and the like). Does Grid care which behavior is used?

Copy link
Member

Choose a reason for hiding this comment

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

How does Grid use this? I don't see it. The Ord instance for Q236 was just intended for using them as keys in maps etc. I'd really rather not use toDouble a <= toDouble b since the whole point of using the Q236 representation is to avoid having to do floating-point comparisons.

Copy link
Member

Choose a reason for hiding this comment

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

I ment Edge, specifically via mkEdgesorry. I haven't gone thought the code so I don't know why it puts it in order. Nvm I see now it's just to have an Eq instance for Polygon.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks to me like all the uses of Ord just treat it as a canonical order, and don't assume that Q236 is an ordered field. I think it's worth adding a comment that this is deliberate, but we should leave the code alone.

@bergey
Copy link
Member Author

bergey commented Sep 19, 2014

@cchalmers Thanks for making nearly all the changes I planned to make today!

bergey added a commit that referenced this pull request Oct 10, 2014
Migrate from `vector-space` package to `linear`

Make types more polymorphic.
@bergey bergey merged commit 6c7f39d into master Oct 10, 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