-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Asdf-Support #708
base: asdf-support
Are you sure you want to change the base?
Asdf-Support #708
Conversation
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 for sharing your work. It looks like a great start! I left a bunch of comments. I did not look over the ndcube.py
changes.
from ndcube.ndcube import NDCube | ||
return [NDCube] | ||
|
||
def select_tag(self, obj, tags, ctx): |
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 would leave this out for now. As you've noted in the comments (which are very helpful!) the ordering here can sometimes lead to confusion. The tag versions can be managed by versioning the extension (in different manifests) so that for a given extension there is only one tag version (and we won't need a select_tag
).
|
||
[project.entry-points."asdf.extensions"] | ||
ndcube = "ndcube.asdf.entry_points:get_extensions" | ||
|
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.
Would you also enable schema tests for the new schemas? Here's the configuration for asdf-astropy
:
https://github.com/astropy/asdf-astropy/blob/1b286108042c1201199e9931cd2a5b52cc57bdce/pyproject.toml#L121-L122
I am not familiar with the ndcube tox and github actions CI but would you look into modifying whatever is necessary there to get the schema tests running for the PR? Let me know if there are more details or anything I can do to help with this. Also, perhaps one of the other maintainers has a sense for how these schema tests might be integrated in the CI.
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.
I don't think this has allowed the schema tests to run in the CI. I think it's related to asdf-format/asdf#1756
The core issue is that the CI run is testing files in the installed package but is running from a temporary directory. Take this test as an example:
../../.tox/py312/lib/python3.12/site-packages/ndcube/extra_coords/tests/test_extra_coords.py::test_empty_ec PASSED
The path to the file contains a bunch of stuff that is very specific to that test run (py312, tox, etc) and this causes the asdf pytest plugin to ignore the schema files since they appear at a similar path which doesn't match the asdf_schema_root
.
As there are test configurations where we might expect the schema tests to fail (like the oldest deps job, see this conversation for more details) we may want to consider adding a new CI job (or jobs) that test just the schemas. Minimally they could be tested against the newest released asdf and perhaps they could also be tested against development versions of asdf and the dependent schema packages. Ignoring the development testing at the moment I think this would involve:
- add a tox factor that runs just the schema tests (defining
asdf_schema_root
, providing a path to the schemas, not running in a temporary directory) - enabling this factor in the CI workflow
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've included a separate environment for schema testing in tox and created a workflow for it. Should I add a separate job for the schema tests as well? Please let me know. (P.S. I got a bit carried away with the workflow concept.)
The schema tests(now enabled) in the CI now doesn't run from a .tmp
directory:
../../ndcube/asdf/resources/manifests/ndcube-0.1.0.yaml . [ 16%]
110
../../ndcube/asdf/resources/schemas/ExtraCoords-0.1.0.yaml . [ 33%]
111
../../ndcube/asdf/resources/schemas/NDCube-0.1.0.yaml . [ 50%]
112
../../ndcube/asdf/resources/schemas/QuantityTableCoordinate-0.1.0.yaml . [ 66%]
113
[ 66%]
114
../../ndcube/asdf/resources/schemas/SkyCoordTableCoordinate-0.1.0.yaml . [ 83%]
115
[ 83%]
116
../../ndcube/asdf/resources/schemas/TimeTableCoordinate-0.1.0.yaml . [100%]
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.
The workflow and tox changes you've included so far look good enough to me. I don't think adding a separate job is necessary (but please let me know if I'm missing something that would provide). Thanks!
tag: "tag:stsci.edu:gwcs/wcs-1.*" | ||
mapping: | ||
type: array | ||
lookup_tables: |
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 am unsure about the structure of the lookup_tables
under the properties validator in the ExtraCoords
schema and would need to test it with more examples. (I'll update it soon)
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
def from_yaml_tree(self, node, tag, ctx): | ||
from ndcube.extra_coords.extra_coords import ExtraCoords | ||
extra_coords = ExtraCoords() | ||
extra_coords._wcs = node.get("wcs") |
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 can't remember if wcs is dynamic if we have lookup tables, so we wouldn't need to serialise it in that case. I can check later.
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.
ExtraCoords can be initialized by a ~astropy.wcs.wcsapi.BaseLowLevelWCS
object and a mapping
. Therfore, I included wcs
and mapping
as a node key-value pairs and when we initialize ExtraCoords with the lookup_tables
, the wcs
is indeed dynamic, and we don't need to serialize it. The structure is such that only when we initialize it with a wcs
object and a mapping
, it will get serialized. Did I understand it correctly, or am I missing something?
pytest | ||
asdf | ||
set_env = | ||
asdf_schema_root = {toxinidir}/ndcube/asdf/resources |
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.
Nice!
This PR adds ASDF support for ndcube objects.
GitHub Project Board Link
This Pull Request is currently a
Work In Progress
and will be continuously updated as we expand support for serializingndcube
objects to ASDF.