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

Add PartialOrder impls #322

Merged
merged 6 commits into from
Mar 9, 2020
Merged

Add PartialOrder impls #322

merged 6 commits into from
Mar 9, 2020

Conversation

frankmcsherry
Copy link
Member

This PR adds PartialOrder implementations for Antichain and AntichainRef. The methods less_equal and less_than clash with the per-time implementations (perhaps those should only be for antichains, which each singleton time is), but the inherent methods take priority in resolution, and this doesn't appear to cause any breakage.

The intent here is to allow these types to implement Lattice in the differential crate, in the interest of tidying up a bunch of bespoke logic.

cc @LorenzSelv

@LorenzSelv
Copy link

That looks good. Maybe we could call the dominates method inside the implementation of less_equal, but it's just a minor thing.

Thanks for taking the time to do this.

@frankmcsherry
Copy link
Member Author

Oh, that's a good idea. I'd be up for deprecating dominates too, with the notes that one should use PartialOrder::less_equal. My guess is that it would be great to get everyone to using the partial order on antichains (with a &[time] for single times), but that would probably be quite breaking without a lot of socialization. I think the users of dominates are mostly in differential.

@LorenzSelv
Copy link

Yeah that would be even better, up to you

@@ -157,6 +164,19 @@ impl<T: PartialOrder> Antichain<T> {
#[inline] pub fn elements(&self) -> &[T] { &self.elements[..] }
}

impl<T: PartialEq> PartialEq for Antichain<T> {
fn eq(&self, other: &Self) -> bool {

Choose a reason for hiding this comment

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

not sure if it works here as I haven't tried, but.. can we just do self.elements() == other.elements() ? slice comparison should do the job.

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 has the defect that it doesn't equate two slices that have the same elements, but in a different order. I think this was one of the reasons that I shied away from the implementation in the first place, but it's probably worth fixing (as otherwise equality testing could return surprising results anyhow).

Choose a reason for hiding this comment

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

I assumed they were sorted, but it's not the case.. also, how can you sort incomparable elements? ahah nvm, dumb comment

@frankmcsherry frankmcsherry merged commit 6471f4b into master Mar 9, 2020
@github-actions github-actions bot mentioned this pull request Oct 29, 2024
@antiguru antiguru deleted the partial_antichains branch October 29, 2024 19:12
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.

2 participants