-
Notifications
You must be signed in to change notification settings - Fork 19
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
Override del_construct
to update cyclic axes set when due
#763
Override del_construct
to update cyclic axes set when due
#763
Conversation
Just going to add one commit to fix a few style/formatting issues detected by the CI linting. |
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.
@davidhassell one note to consider for your review. Thanks.
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.
Hi Sadie - look good - I've made some suggestions on handling the construct-doesn't exist case.
Thanks for your feedback @davidhassell. I'll update the PR shortly according to your excellent suggestions, though I will get the halo PR review in first, so do it after that. |
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
All ready for your re-review, @davidhassell. Thanks. |
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.
Look great. Thanks!
Thanks David. In that case, merging! |
Fix #758 and #756 (the former being the underlying issue and the latter a specific bug as a downstream effect) by overriding the
del_construct
inherited from cfdm to account for and update the store ofcyclic
axes. Previously we used the inherited method as-is so cyclic axes weren't updated and could get out of sync, notably contain axes which no longer existed due to deletion.Note:
test_Domain_del_construct
added here will currently fail halfway-through due tocyclic
: different sets forField.domain
& correspondingField
#762, as noted in the test. I am fixing that next.