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

Add envelopeS / envelopeSMay for querying scalar displacements from envelopes #31

Merged
merged 4 commits into from Nov 2, 2012

Conversation

Projects
None yet
2 participants
@mgsloan
Member

mgsloan commented Oct 30, 2012

I realize these shouldn't be encouraged, but they seem potentially useful!

Discussing the "intrudeEnvelope" in the haddocks led to figuring out how to express surprise at the fact that "envelopeS v x - envelopeS (negateV v) x < 0" is possible.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 30, 2012

Member

I find the use of the subjunctive and the discussion of "moving" the bounding hyperplane confusing. Isn't this just returning the magnitude of whatever is returned by envelopeV[May]? If so, the wording of the documentation ought to be similar as well.

Also, there seems to be a type error -- see the little red dot next to 3ce357b for a link to the Travis build log.

Member

byorgey commented Oct 30, 2012

I find the use of the subjunctive and the discussion of "moving" the bounding hyperplane confusing. Isn't this just returning the magnitude of whatever is returned by envelopeV[May]? If so, the wording of the documentation ought to be similar as well.

Also, there seems to be a type error -- see the little red dot next to 3ce357b for a link to the Travis build log.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Nov 2, 2012

Member

Looks good to me. Just to be clear, note that according to this patch, these new functions are exported from Diagrams.Core.Envelope but not re-exported from Diagrams.Core. I think that's an acceptable state of affairs, given how (as you say) these "shouldn't be encouraged" (though I don't know if you intended it or just didn't think to add exports to Diagrams.Core). However, I could be convinced to re-export them. In any case, I'll go ahead and merge and the question of re-exporting can be dealt with later.

Member

byorgey commented Nov 2, 2012

Looks good to me. Just to be clear, note that according to this patch, these new functions are exported from Diagrams.Core.Envelope but not re-exported from Diagrams.Core. I think that's an acceptable state of affairs, given how (as you say) these "shouldn't be encouraged" (though I don't know if you intended it or just didn't think to add exports to Diagrams.Core). However, I could be convinced to re-export them. In any case, I'll go ahead and merge and the question of re-exporting can be dealt with later.

byorgey added a commit that referenced this pull request Nov 2, 2012

Merge pull request #31 from mgsloan/envelope_scalar
Add envelopeS / envelopeSMay for querying scalar displacements from envelopes

@byorgey byorgey merged commit 2606f13 into diagrams:master Nov 2, 2012

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