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

return list of traces #48

Merged
merged 14 commits into from Jan 14, 2014

Conversation

Projects
None yet
2 participants
@jeffreyrosenbluth
Member

jeffreyrosenbluth commented Jan 2, 2014

Hopefully that does it but I haven't tested the rayTrace… functions yet.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 2, 2014

Member

We should remove PosInf. We used to use +infinity to indicate no intersections; that should now be indicated by the empty list.

Member

byorgey commented on src/Diagrams/Core/Trace.hs in 6d60fce Jan 2, 2014

We should remove PosInf. We used to use +infinity to indicate no intersections; that should now be indicated by the empty list.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 2, 2014

Member

This is an inefficient implementation (O(n^2) rather than O(n)) and will not play nicely with calls to head and so on. We should require as an invariant that both lists are sorted, then we can write a merge operation which walks through both lists in parallel.

In fact, perhaps we should make a SortedList newtype and give it Semigroup and Monoid instances.

Member

byorgey commented on src/Diagrams/Core/Trace.hs in 6d60fce Jan 2, 2014

This is an inefficient implementation (O(n^2) rather than O(n)) and will not play nicely with calls to head and so on. We should require as an invariant that both lists are sorted, then we can write a merge operation which walks through both lists in parallel.

In fact, perhaps we should make a SortedList newtype and give it Semigroup and Monoid instances.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 2, 2014

Member

Should be []

Member

byorgey commented on src/Diagrams/Core/Trace.hs in 6d60fce Jan 2, 2014

Should be []

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

I think it should be possible to attach this deriving clause directly to the definition of Trace.

Member

byorgey commented on src/Diagrams/Core/Trace.hs in be67bc1 Jan 3, 2014

I think it should be possible to attach this deriving clause directly to the definition of Trace.

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 3, 2014

Member

So newtype Trace v ... becomes newtype (Ord (Scalar v)) => Trace v …

Member

jeffreyrosenbluth replied Jan 3, 2014

So newtype Trace v ... becomes newtype (Ord (Scalar v)) => Trace v …

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

I don't think you need the Ord (Scalar v) part, I think if you attach the deriving clause to the declaration it infers the constraint on the Semigroup instance. I could be wrong though. If it's a choice between adding that constraint on the newtype and using a standalone deriving clause we should definitely go with the standalone.

Member

byorgey replied Jan 3, 2014

I don't think you need the Ord (Scalar v) part, I think if you attach the deriving clause to the declaration it infers the constraint on the Semigroup instance. I could be wrong though. If it's a choice between adding that constraint on the newtype and using a standalone deriving clause we should definitely go with the standalone.

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 3, 2014

Member

Nope, it does not infer the constraint.
I leave it as standalone.

Member

jeffreyrosenbluth replied Jan 3, 2014

Nope, it does not infer the constraint.
I leave it as standalone.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

This comment doesn't make sense anymore, it should say something like "the empty trace".

Member

byorgey commented on src/Diagrams/Core/Trace.hs in be67bc1 Jan 3, 2014

This comment doesn't make sense anymore, it should say something like "the empty trace".

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 3, 2014

Member

I think this is slightly easier to read

Member

jeffreyrosenbluth commented on 4f56513 Jan 3, 2014

I think this is slightly easier to read

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

Agreed, though why not use appTrace instead of op Trace?

Member

byorgey replied Jan 3, 2014

Agreed, though why not use appTrace instead of op Trace?

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 3, 2014

Member

I guess I feel that we should use our lens accessors whenever we can and that we left in the projections mostly for backwards compatibly. Also using op <Constructor> consistently means I don't have to remember newtype record field names. I think this is probably ready to merge now.

Member

jeffreyrosenbluth replied Jan 3, 2014

I guess I feel that we should use our lens accessors whenever we can and that we left in the projections mostly for backwards compatibly. Also using op <Constructor> consistently means I don't have to remember newtype record field names. I think this is probably ready to merge now.

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

OK, I'm fine with that.

Member

byorgey replied Jan 3, 2014

OK, I'm fine with that.

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 3, 2014

Member

We left in some of the projections not just for backwards compatibility, but because they are semantically meaningful operations on their own. appTrace means "turn a trace into a function I can apply", which is a meaningful operation on traces. The fact that this so happens to be a newtype projection is just an implementation detail; one can imagine other implementations of trace where we would still have a function called appTrace with the same type signature, which is not a simple projection. In my opinion, users should be able to use the Trace API without actually knowing that it is a newtype. SortedList, on the other hand, is manifestly a newtype wrapper so using lenses to wrap and unwrap is the API. Anyway, not trying to argue, I don't think it's that important. Was just thinking about this and thought it was worth elaborating a bit.

I have a few more changes I want to make and then I'll merge if you're OK with them.

Member

byorgey replied Jan 3, 2014

We left in some of the projections not just for backwards compatibility, but because they are semantically meaningful operations on their own. appTrace means "turn a trace into a function I can apply", which is a meaningful operation on traces. The fact that this so happens to be a newtype projection is just an implementation detail; one can imagine other implementations of trace where we would still have a function called appTrace with the same type signature, which is not a simple projection. In my opinion, users should be able to use the Trace API without actually knowing that it is a newtype. SortedList, on the other hand, is manifestly a newtype wrapper so using lenses to wrap and unwrap is the API. Anyway, not trying to argue, I don't think it's that important. Was just thinking about this and thought it was worth elaborating a bit.

I have a few more changes I want to make and then I'll merge if you're OK with them.

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 3, 2014

Member

Sounds good

btw you make some good points about appTrace and I'm fine it you want to use it instead of op Trace. It's partly a matter of style and I don't think its that important either.

Member

jeffreyrosenbluth commented Jan 3, 2014

Sounds good

btw you make some good points about appTrace and I'm fine it you want to use it instead of op Trace. It's partly a matter of style and I don't think its that important either.

Brent Yorgey
updates to SortedList
  * Remove Wrapped instance.  Part of the point is that SortedList a
    is NOT isomorphic to [a].
  * Add smart constructor, accessor, and some mapping functions
  * other misc refactorings
@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 12, 2014

Member

Ahha, good - i was wondering where to maintain the invariant.

Member

jeffreyrosenbluth commented on 096eff8 Jan 12, 2014

Ahha, good - i was wondering where to maintain the invariant.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Jan 13, 2014

Member

@jeffreyrosenbluth , what do you think of my changes? You can see the generated Haddock documentation (with diagrams-haddock images) here: http://www.cis.upenn.edu/~byorgey/diagrams-core/Diagrams-Core-Trace.html .

Other than the SortedList changes which you already saw, I was just making improvements to the documentation.

Member

byorgey commented Jan 13, 2014

@jeffreyrosenbluth , what do you think of my changes? You can see the generated Haddock documentation (with diagrams-haddock images) here: http://www.cis.upenn.edu/~byorgey/diagrams-core/Diagrams-Core-Trace.html .

Other than the SortedList changes which you already saw, I was just making improvements to the documentation.

byorgey added a commit that referenced this pull request Jan 14, 2014

@byorgey byorgey merged commit 11d5e23 into master Jan 14, 2014

1 check failed

default The Travis CI build failed
Details

@byorgey byorgey deleted the traces branch Jan 14, 2014

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Jan 14, 2014

Member

@bergey - you should be able to fix your clipTo algorithm now.

Member

jeffreyrosenbluth commented Jan 14, 2014

@bergey - you should be able to fix your clipTo algorithm now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment