-
Notifications
You must be signed in to change notification settings - Fork 289
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
Conversation
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. |
If we do get to it then we'll need a rebase onto main before it can be reviewed I think |
There was a problem hiding this 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
.
Rebased. It is not good to go, since the auto-merged result is still using Coord._as_defn() |
Code now simplified ... |
@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. |
Thanks @pp-mo! I'll let @stephenworsley take it over the line. |
There was a problem hiding this 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
Thanks @stephenworsley |
There was a problem hiding this 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.
Thanks @stephenworsley |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Oh, I think this might want a whatsnew also.
There was a problem hiding this 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.
Whoops sorry forgot. |
Hang on now I need to rebase/resolve... |
Ok, rebased with added whatsnew
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Thanks @stephenworsley for going the distance ! |
Thanks guys! 😀 |
Aiming to address #3616