Add support for masked quantities, angles, and coordinates

[lpsinger]

Fixes #202.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (b36f1ab) to head (c07b3df).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   99.40%   99.42%   +0.01%     
==========================================
  Files          62       64       +2     
  Lines        2200     2254      +54     
==========================================
+ Hits         2187     2241      +54     
  Misses         13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[braingram]

Thanks for opening this PR.

The unit tests are failing because this introduces an import of astropy at "integration" (when the extension/entry point is loaded). The asdf docs have more details on how this can lead to performance issues:
https://asdf.readthedocs.io/en/latest/asdf/extending/converters.html#entry-point-performance-considerations
but the tldr; is that we can't import astropy when the Converter/Extension is created. That's the reason there are local imports in the Converter methods like this one:

asdf-astropy/asdf_astropy/converters/coordinates/angle.py

Line 25 in 6826f49

from astropy.coordinates.angles import Latitude

This makes supporting Masked complicated. Is there a public API endpoint for the generated Masked subclasses? If not things are going to be even more complicated since we also can't use the path to the expected class. The class factory pattern leads to complicated situations like:

>> import astropy.utils.masked as masked
>> import astropy.units as u
>> MQ = masked.Masked(u.Quantity)
>> MQ
astropy.utils.masked.core.MaskedQuantity
>> masked.core.MaskedQuantity
AttributeError: module 'astropy.utils.masked.core' has no attribute 'MaskedQuantity'

I read this as masked stating that the MaskedQuantity class exists at astropy.utils.masked.core.MaskedQuantity but it doesn't. For asdf to check for instances to handle with the Converter it needs to have access to the class.

[lpsinger]

I read this as masked stating that the MaskedQuantity class exists at astropy.utils.masked.core.MaskedQuantity but it doesn't. For asdf to check for instances to handle with the Converter it needs to have access to the class.

That's right. Masked is apparently a class factory, and types are dynamically created here: https://github.com/astropy/astropy/blob/v7.0.0/astropy/utils/masked/core.py#L266-L271

The fully qualified name astropy.utils.masked.core.MaskedQuantity seems to be totally fictional because it is not added to the module dictionary. Even if it wasn't, I don't see how that fully qualified name would distinguish between instances of the class Masked(astropy.units.Quantity) vs Masked(foo.bar.Quantity) (a class with a name that is coincidentally the same but comes from a completely different module).

Is there a way to configure a converter to decide whether it is capable of handling instances of a class by calling a function?

[braingram]

Is there a way to configure a converter to decide whether it is capable of handling instances of a class by calling a function?

Not that I'm aware of. The matching of converter to object uses the object type:
https://github.com/asdf-format/asdf/blob/a0898c2d8d5afad0f8cf9a0d60847263d786f9e5/asdf/yamlutil.py#L297
and the assumption is that either:

• asdf has indexed the class (it was provided via Converter.tags as is done in this PR at the expense of importing the required modules to make the class at "integration" time)
• asdf has the "path" via which the class can be imported (so asdf can check if the contained module is imported and index the converter as needed)

(see this comment for more details).

I suspect that more extensive changes to asdf will be needed to support Masked since the classes are not publicly importable via astropy (we can't provide a "path" to the class in Converter.tags).

[lpsinger]

I suspect that more extensive changes to asdf will be needed to support Masked since the classes are not publicly importable via astropy (we can't provide a "path" to the class in Converter.tags).

What do you have in mind?

It is turning out to be surprisingly simple to implement Masked classes; the significant hurdle is how to register the classes without importing them.

[lpsinger]

One option could be to have Astropy add those dynamically-created subclasses to the module dictionary, so that (for example) Masked(astropy.units.Quantity) is available as astropy.utils.masked.Masked_astropy_units_Quantity (would need some way to transform the fully qualified name of the data class so that it is a valid Python identifier).

[braingram]

I suspect that more extensive changes to asdf will be needed to support Masked since the classes are not publicly importable via astropy (we can't provide a "path" to the class in Converter.tags).

What do you have in mind?

I will try to carve out some time to look into this soon. At the moment I don't have a clear solution to propose.

[lpsinger]

I suspect that more extensive changes to asdf will be needed to support Masked since the classes are not publicly importable via astropy (we can't provide a "path" to the class in Converter.tags).

What do you have in mind?

I will try to carve out some time to look into this soon. At the moment I don't have a clear solution to propose.

For a possible workaround, see astropy/astropy#17685.

[pllim]

Are you able to run a job here using your branch at astropy/astropy#17685 and see if it is actually green? Thanks! (devdeps is usually the easiest to hack for such things)

[lpsinger]

Are you able to run a job here using your branch at astropy/astropy#17685 and see if it is actually green? Thanks! (devdeps is usually the easiest to hack for such things)

... maybe? See latest commit 🤞

[lpsinger]

Oh right, your fork won't have the right tags for setuptools_scm. You can make a fake tag on your fork, or otherwise relax any astropy pins here. The former might be easier.

I'm not really sure how to make this work in the CI against a branch of astropy. But the tests I added are passing on my computer.

[pllim]

astropy==0.2.dev37528+g4b8a62975

Try grab v7.1.dev tag from astropy proper and push that tag to your fork. Then rerun the jobs.

[pllim]

https://github.com/astropy/astropy/releases/tag/v7.1.dev

[braingram]

dfee919
is one way to appease (parts of) the CI. With that change the tests (added in this PR) are passing with the astropy PR:
https://github.com/astropy/asdf-astropy/actions/runs/13018230750/job/36312572977?pr=254
@lpsinger would you integrate that change with this PR (and drop the tox changes). Not everything will pass (the py310 jobs will fail with astropy dev which doesn't support python 3.10) but hopefully it will illustrate that the astropy changes make this PR possible.

[lpsinger]

Tests are passing now, except for Python 3.10 which is EOL as far as Astropy is concerned.

[braingram]

Overall this looks good to me. Once the astropy PR is merged we can:

• restore the pyproject.toml astropy version
• update this PR with the appropriate version checks to get things working with all supported (and dev) astropy versions

Thanks again for the PR and adding support for Masked classes!