Skip to content

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