fix serialization of bounding_box with quantities#8052
Conversation
|
Hi there @nden 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
|
Does this close or complement #8037? Does this need a test? |
f2a7307 to
40fd626
Compare
astrofrog
left a comment
There was a problem hiding this comment.
Looks good but just one small comment - I'm approving so that this can be merged once that small comment is addressed (or not if no changes are needed)
| model.bounding_box = cls._domain_to_bounding_box(node['domain']) | ||
| elif 'bounding_box' in node: | ||
| model.bounding_box = node['bounding_box'] | ||
| if 'bounding_box' in node: |
There was a problem hiding this comment.
Why the change from elif to if? If you want the 'bounding_box' clause to take precedence, maybe put it first and then use an elif?
There was a problem hiding this comment.
I was considering removing domain. It was replaced by bounding_box quite a while ago.
@drdavella Is there any reason NOT to remove domain? I believe it was only used by jwst and by now there shouldn't be any files left with it.
There was a problem hiding this comment.
@nden it's okay with me to remove. It was removed in v1.1.0 of the standard, so there's no requirement to support backwards compatibility here given that the version of TransformType is already pinned to 1.1.0.
There was a problem hiding this comment.
After more conversations locally - removing it now doesn't appear to be a problem.
|
Comment has been addressed, so I'm merging this one, too. |
|
Thank you @nden! |
fix serialization of bounding_box with quantities
fix serialization of bounding_box with quantities (cherry picked from commit d78b3cb)
Fixes issue brought up in #8037.