Skip to content

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

}

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