Skip to content
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

Allow serializing and deserializing named astropy units #2475

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

Carifio24
Copy link
Member

This PR adds serialization support for astropy NamedUnits. The immediate use case for this is storing the decay unit in glue-viz/glue-wwt#103. This seems to work well for me, and it looks like astropy will give the builtin instance after deserialization, e.g.

import astropy.units as u
assert u.Unit("m") is u.m  # Passes

However, if someone more astropy-savvy than myself knows of a better way that we should do this, please let me know!

Copy link
Collaborator

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this should be functional for the basic use case. However one would run into problems as soon as any CompoundUnit comes into play, which is not a subclass of NamedUnit (or even Unit).
Those two are probably the most relevant cases in practice, so one might also add just another saver for CompoundUnit, but setting it up directly for UnitBase seems to do the job just as well, and I currently see no risk in enabling serialisation for them all.

Note that for CompoundUnit this does not guarantee exact identity (unit2 is unit) and probably is not realistic to achieve, since to_str will always reduce to a default form, but I think this should be sufficient for practical use?

@@ -65,6 +65,7 @@ def load(rec, context)
import numpy as np
from matplotlib.colors import Colormap
from matplotlib import cm
from astropy.units import NamedUnit, Unit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from astropy.units import NamedUnit, Unit
from astropy.units import Unit, UnitBase

Comment on lines 625 to 632
@saver(NamedUnit)
def _save_named_unit(unit, context):
return dict(named_unit=unit.to_string())


@loader(NamedUnit)
def _load_named_unit(rec, context):
return Unit(rec["named_unit"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@saver(NamedUnit)
def _save_named_unit(unit, context):
return dict(named_unit=unit.to_string())
@loader(NamedUnit)
def _load_named_unit(rec, context):
return Unit(rec["named_unit"])
@saver(UnitBase)
def _save_unit_base(unit, context):
return dict(unit_base=unit.to_string())
@loader(UnitBase)
def _load_unit_base(rec, context):
return Unit(rec["unit_base"])

glue/core/tests/test_state.py Show resolved Hide resolved
@Carifio24
Copy link
Member Author

@dhomeier Thanks for the review! My main motivation for ensuring exact identity was making sure that unit conversions would work correctly. I wasn't really sure what sorts of unit classes were out there so I didn't want to overreach and cause something to break in the future on some unit class that I hadn't prepared for.

I wasn't aware of CompositeUnit but after some experimentation it seems like that's not a concern there, regular equality is fine. I've updated things to match your suggestions.

@dhomeier
Copy link
Collaborator

Admittedly I have not checked all examples of exotic units one might encounter here.
There could be problems with

  1. Units that are only available after enabling a specific system, like u.mile in u.imperial.
  2. vounit format on top of that even allows defining arbitrary custom units that could shadow standard units.

Currently those cases would probably simply fail on creating u.Unit("furlong") in the loader, but perhaps it makes more sense to add a sanity check already in the saver:

# Check that unit can be parsed back with default enabled systems
try:
    with u.set_enabled_units([u.si, u.cgs, u.astrophys]):
        saved_unit = u.Unit(unit.to_string())
except ValueError:
    raise GlueSerializeError(f"Serializing units of '{unit}' is not yet supported")

(a bit complicated since some custom system might already have been added in the saver context, but not be available when reading back).

@Carifio24
Copy link
Member Author

@dhomeier I think the sanity check is a good idea. I'm trying to imagine what it would look like for a plugin/config to overwrite this kind of behavior in a straightforward way.

I'm wondering if it would be better to give a warning rather than raise an error. It seems like non-default units (e.g. the units in imperial) aren't generally distinguishable by their type (which is what the glue serializer/deserializer use for dispatch), so I'm trying to think what would be a good way to easily overwrite/extend the serialization behavior for these units without having to do something like the following, which feels clunky:

original_loader = GlueUnSerializer.dispatch[UnitBase][0]
@loader(UnitBase)
def _updated_unit_loader(rec, context):
    if is_one_of_my_non_default_units(rec["unit_base"]):
        # Return my custom loaded unit
    return original_loader(rec, context)

and similarly in the saver. Regardless, I think it's going to be incumbent on anything that implements a custom saver to implement a custom loader, and vice versa.

Or maybe I'm overthinking things and we should just raise an error as you suggested.

@Carifio24
Copy link
Member Author

@dhomeier I've updated this to make the changes that you suggested.

astrofrog and others added 2 commits April 11, 2024 14:47
Co-authored-by: Derek Homeier <709020+dhomeier@users.noreply.github.com>
Copy link
Member

@astrofrog astrofrog 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 to me! I think raising an error is fine but let's revisit if this ends up causing issues.

@astrofrog astrofrog merged commit af311e0 into glue-viz:main Apr 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants