-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add serialization logic for uncertainty objects #239
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 99.44% 99.45% +0.01%
==========================================
Files 54 57 +3
Lines 1994 2037 +43
==========================================
+ Hits 1983 2026 +43
Misses 11 11 ☔ View full report in Codecov by Sentry. |
@braingram, @Cadair, could you guys please review it. |
caa3d46
to
837fcbb
Compare
Recent commits also added |
3418339
to
09df40a
Compare
@@ -6,15 +6,15 @@ description: |- | |||
model classes, which are handled by an implementation of the ASDF | |||
transform extension. | |||
asdf_standard_requirement: | |||
gte: 1.6.0 |
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.
Same as #246 (comment)
This bumps the "dev" manifest to 1.3.0.
@Cadair I can't request you as a reviewer. Would you give this a review? I think this one looks good although given that I padded a couple commits an independent review would be helpful. |
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 other than I think it could be simplified.
@@ -0,0 +1,20 @@ | |||
%YAML 1.1 |
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.
Given the near identical nature of these three schemas, have you considered just using one tag and schema to cover all three cases and making the type of uncertainty an attribute in the schema?
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.
Thanks. I think using a single tag would require putting some sort of class identifier (either a name, code, or converted name) in the data files (unless I'm missing something) to know what class to use for deserialization. I think there are a few downsides to this.
If we use the class name we're further tying the ASDF representation to the astropy API. If astropy decides to rename a class we would have to retain some mapping of old name to new name. By using a different tag per class we would only have to update the converter to add the new class name to the types (which we would also have to do if we used the other approach).
If we create a mapping of class name to converted class name (for example StdDevUncertainty
to stdev
) we will be less dependent on the astropy API (so a class name change would only require changing the converter). However if a new uncertainty is added we would need to bump the tag version and start managing a different mapping for each tag version. If we use separate tags and astropy adds a new uncertainty we can just add a new tag converter and schema for the new uncertainty (and don't have to make any changes to the other schemas and converters).
I think using separate tags allows us to more fully use the existing asdf extension/tag versioning features (albeit with duplication in the schemas).
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.
It doesn't have to be an explicit class name, just a descriptive attribute (e.g., uncertainty_type = unknown
, variance
, or stddev
). from_yaml_tree
then can check the value of this attribute and call the appropriate constructor. That would keep it generic enough for any language library.
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.
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.
Yes, this is exactly what I had in mind. I think it makes it easier to add new uncertainty types as well.
@@ -0,0 +1,45 @@ | |||
class _UncertaintyBaseConverter: |
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 think this could all be done by one converter if you took the suggestion later to handle all this with one tag.
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.
LGTM
Fixes: #238
This PR adds the support for the serialization of below objects to ASDF
astropy.nddata.nduncertainty.StdDevUncertainty
astropy.nddata.nduncertainty.UnknownUncertainty