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

Implemented constraint equality. #3749

Merged
merged 6 commits into from
Nov 17, 2022
Merged

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Jul 2, 2020

Aiming to address #3616

@pp-mo pp-mo mentioned this pull request Jul 2, 2020
@rcomer rcomer linked an issue Jul 3, 2020 that may be closed by this pull request
@trexfeathers trexfeathers added this to the v3.1.0 milestone Aug 21, 2020
@bjlittle bjlittle modified the milestones: v3.1.0, v3.2.0 Aug 4, 2021
@jamesp jamesp modified the milestones: v3.2.0, v3.3.0 Oct 28, 2021
@wjbenfold wjbenfold self-requested a review January 11, 2022 17:20
@wjbenfold wjbenfold self-assigned this Jan 11, 2022
@lbdreyer
Copy link
Member

Although this is technically in progress, it isn't necessarily going to go into Iris 3.2. So to avoid it cluttering the "in progress" column, I will move it to to do. We can then pick it up if we time.

@wjbenfold
Copy link
Contributor

If we do get to it then we'll need a rebase onto main before it can be reviewed I think

@trexfeathers trexfeathers removed this from the v3.3.0 milestone Sep 27, 2022
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

I'm happy with this approach. I think the important thing here is that no two constraints with different behaviour can show up as equal.

I think the only changes needed here are changing references from defn to metadata.

@trexfeathers trexfeathers assigned pp-mo and unassigned stephenworsley Nov 9, 2022
@trexfeathers
Copy link
Contributor

@pp-mo are you able to finish this one off before getting back into #5016? Sounds like this is pretty close.

@pp-mo pp-mo marked this pull request as draft November 11, 2022 15:45
@pp-mo
Copy link
Member Author

pp-mo commented Nov 11, 2022

Rebased.
but N.B. put in draft temporarily

It is not good to go, since the auto-merged result is still using Coord._as_defn()
And we now want to use common metadata instead.

@pp-mo
Copy link
Member Author

pp-mo commented Nov 11, 2022

Code now simplified ...
On closer inspection, I conclude that the 'coord_name' of a _CoordConstraint is only ever intended to be a string.
The only way it can be otherwise is if you construct Constraint(coord_values={something:value}). But whereas my earlier code suggested that 'something' could be a CoordDefn (or, now, a metadata), it actually can't since that is not a hashable type, so cannot act as a dict key.
It is still just about "possible" that you could pass a Coord instance as the identifier in this case, but we have no tests for it.
I simply don't believe it is useful, so I'm no longer supporting it as a special case.

@pp-mo pp-mo marked this pull request as ready for review November 11, 2022 16:59
@pp-mo
Copy link
Member Author

pp-mo commented Nov 11, 2022

@trexfeathers I think this is good to go. Stephen's comment with change request is outstanding, but I think you can dismiss that if you think it is now OK.

@trexfeathers
Copy link
Contributor

Thanks @pp-mo! I'll let @stephenworsley take it over the line.

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looking good, just a few minor changes and a whatsnew entry and I think this should be good to go

@pp-mo
Copy link
Member Author

pp-mo commented Nov 16, 2022

Thanks @stephenworsley
I hope this latest addresses all the points raised.
Sorry for the delay !

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looking good, just some very minor changes and I think we're good to go.

@pp-mo
Copy link
Member Author

pp-mo commented Nov 16, 2022

Thanks @stephenworsley
I think I've now fixed them all to be the same.

stephenworsley
stephenworsley previously approved these changes Nov 16, 2022
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good!

@stephenworsley stephenworsley dismissed their stale review November 16, 2022 17:04

Oh, I think this might want a whatsnew also.

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

I forgot to mention before, but this still needs a whatsnew entry.

@pp-mo
Copy link
Member Author

pp-mo commented Nov 16, 2022

I forgot to mention before, but this still needs a whatsnew entry.

Whoops sorry forgot.
Doing it now ...

@pp-mo
Copy link
Member Author

pp-mo commented Nov 16, 2022

Hang on now I need to rebase/resolve...

@pp-mo
Copy link
Member Author

pp-mo commented Nov 16, 2022

Ok, rebased with added whatsnew

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good!

@stephenworsley stephenworsley merged commit 2ed9b7e into SciTools:main Nov 17, 2022
@pp-mo pp-mo deleted the constraint_eq branch November 17, 2022 10:50
@pp-mo
Copy link
Member Author

pp-mo commented Nov 17, 2022

Thanks @stephenworsley for going the distance !

@rcomer
Copy link
Member

rcomer commented Nov 17, 2022

Thanks guys! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Constraint equality?
8 participants