Initial import of the package #2

nden · 2020-06-16T22:13:37Z

This is the initial import of the package working with the new converters. The new extension (provider) is set up, as well as the converters in basic.py and polynomial.py. Works for me locally - example with Chebyshev1D:

#ASDF 1.0.0
#ASDF_STANDARD 1.5.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.1.0
asdf_library: !core/software-1.0.0 {author: Space Telescope Science Institute, homepage: 'http://github.com/spacetelescope/asdf',
  name: asdf, version: 2.6.1.dev59+gde25fec.d20200615}
history:
  extensions:
  - !core/extension_metadata-1.0.0
    extension_class: asdf.extension.BuiltinExtension
    software: !core/software-1.0.0 {name: asdf, version: 2.6.1.dev59+gde25fec.d20200615}
m: !<http://asdf-format.org/schemas/transform/ortho_polynomial-2.0.0>
  coefficients: !core/ndarray-1.0.0
    data: [0.0, 0.0]
    datatype: float64
    shape: [2]
  polynomial_type: chebyshev
  window: [-1, 1]
...

eslavich

I left some commentary on the subset of files that have been, er, converted to converters. Glad to see it working!

eslavich · 2020-06-17T13:17:44Z

asdf_astropy/tags/transform/basic.py

+
+
+class TransformType(asdf.AsdfConverter):
+    tags = {"http://asdf-format.org/schemas/transform/transforms-1.0.0"}


I would just leave out tags and types here, since we don't want to actually write a generic transform to a file.

eslavich · 2020-06-17T13:18:03Z

asdf_astropy/tags/transform/basic.py

+class TransformType(asdf.AsdfConverter):
+    tags = {"http://asdf-format.org/schemas/transform/transforms-1.0.0"}
+    types = {Model}
+    version = '2.0.0'


The version and requires attributes are no longer recognized by the framework

eslavich · 2020-06-17T13:23:12Z

asdf_astropy/tags/transform/basic.py

+        node = self.to_tree_transform(model)
+        return self._to_tree_base_transform_members(model, node)
+
+    def assert_equal(self, a, b):


These assert_equal methods are something that I haven't given much thought yet. It would be nice to put the responsibility of defining equality onto astropy itself, where upkeep will be easier.

eslavich · 2020-06-17T13:27:21Z

asdf_astropy/tags/transform/basic.py

+
+
+class IdentityType(TransformType):
+    tags = {"http://asdf-format.org/schemas/transform/identity-2.0.0"}


We'll still need to support the old tags, so this list should also include:

"tag:stsci.edu:asdf/transform/identity-1.0.0", "tag:stsci.edu:asdf/transform/identity-1.1.0", "tag:stsci.edu:asdf/transform/identity-1.2.0",

For transforms whose schemas see a lot of changes in this migration, we may want to maintain legacy support in a separate class, just so things don't get confusing.

eslavich · 2020-06-17T13:27:42Z

asdf_astropy/tags/transform/basic.py

+
+class ConstantType(TransformType):
+    tags = {"http://asdf-format.org/schemas/transform/constant-2.0.0"}
+    supported_versions = ['1.0.0', '1.1.0', '1.2.0', '1.3.0', '1.4.0', '2.0.0']


supported_versions is also now ignored, that is determined by the list of tags.

eslavich · 2020-06-17T13:29:06Z

asdf_astropy/tags/transform/basic.py

+    types = {models.Const1D, models.Const2D}
+
+    def from_yaml_tree(self, node):
+        if self.version < AsdfVersion('1.4.0'):


The self.version property still exists, though at the moment it returns a string, I should probably change that.

(I'm not a fan of AsdfVersion and hope to deprecate it)

eslavich · 2020-06-17T13:32:40Z

asdf_astropy/tags/transform/basic.py

+
+
+class UnitsMappingType(asdf.AsdfConverter):
+    tags = {"http://asdf-format.org/schemas/transform/units_mapping-2.0.0"}


This transform schema was deemed to be too astropy-specific for the general use transforms, so it's actually still living in astropy with URI http://astropy.org/schemas/astropy/transform/units_mapping-1.0.0.

Can a third party implementation of the standard implement only part of the schemas? If so, perhaps we should keep all schemas in one place and let people decide what they want to implement. I think it's something we should discuss further.

That sounds like a good idea to me, there are so many transforms that the prospect of implementing them all is intimidating.

I wonder if we should separate them into namespaces to guide people on implementing a minimal set? transform/core and others.

This worries me as well. I don't think people should be forced to implement them all (e.g., all the projection transform)

eslavich · 2020-06-17T13:37:23Z

asdf_astropy/tags/transform/polynomial.py

+                assert a.y_window == b.y_window
+
+
+class PolynomialType1_0(PolynomialTypeBase):


Ha! I forgot about these. I did this because I didn't understand how supported_versions worked in the old framework. We can merge these back into one class again and just list multiple tags.

eslavich · 2020-06-17T13:44:40Z

asdf_astropy/extension.py

+    converter_classes = list(_asdf_format_types)
+
+
+asdf.configure(converter_providers=[TransformConverterProvider()])


I don't think we want to call configure from within a library, since that will override other providers that the user might have installed. The entry point should take care of this for us (but maybe it's broken right now, I can't remember if I tested that specifically).

A quick test shows it doesn't work without it.

Ah okay thanks, I'll investigate

I think I found the issue, left a comment here:

https://github.com/astropy/asdf-astropy/pull/2/files#r441786043

eslavich · 2020-06-17T19:34:08Z

setup.cfg

+    asdf-transform-schemas
+
+[options.entry_points]
+asdf_extensions =


The name of the entry point is different for the new-style plugin. This should be:

asdf.converter_providers = asdf-astropy = asdf_astropy.extension:TransformConverterProvider

perrygreenfield · 2020-06-26T15:38:57Z

asdf_astropy/tags/transform/basic.py

+__all__ = ['TransformType', 'IdentityType', 'ConstantType']
+
+
+class TransformType(asdf.AsdfConverter):


Instead change the class names to use Converter instead of Type?

eslavich

One detail I noticed in passing, but otherwise this looks ready to go

eslavich · 2020-06-29T14:12:38Z

asdf_astropy/tags/transform/compound.py

+            "tag:stsci.edu:asdf/transform/fix_inputs-1.2.0",
+            }
+    types = {CompoundModel}
+    handle_dynamic_subclasses = True


This attribute controls a feature that we're planning to remove (and not implement in AsdfConverter), so we can drop it.

eslavich reviewed Jun 17, 2020

View reviewed changes

nden force-pushed the converter-initial branch from 8118263 to ef99f4d Compare June 26, 2020 13:19

nden changed the title ~~WIP: Converter initial~~ Initial import of the package Jun 26, 2020

nden requested a review from perrygreenfield June 26, 2020 13:37

perrygreenfield reviewed Jun 26, 2020

View reviewed changes

nden force-pushed the converter-initial branch from 030017c to 5697030 Compare June 26, 2020 20:03

eslavich approved these changes Jun 29, 2020

View reviewed changes

perrygreenfield approved these changes Jun 29, 2020

View reviewed changes

initial import

9ea018a

nden force-pushed the converter-initial branch from 5697030 to 9ea018a Compare June 29, 2020 16:59

nden merged commit 28487c1 into astropy:master Jun 29, 2020



		class TransformType(asdf.AsdfConverter):
		tags = {"http://asdf-format.org/schemas/transform/transforms-1.0.0"}



		class IdentityType(TransformType):
		tags = {"http://asdf-format.org/schemas/transform/identity-2.0.0"}



		class UnitsMappingType(asdf.AsdfConverter):
		tags = {"http://asdf-format.org/schemas/transform/units_mapping-2.0.0"}

		assert a.y_window == b.y_window


		class PolynomialType1_0(PolynomialTypeBase):

		converter_classes = list(_asdf_format_types)


		asdf.configure(converter_providers=[TransformConverterProvider()])

		__all__ = ['TransformType', 'IdentityType', 'ConstantType']


		class TransformType(asdf.AsdfConverter):

Uh oh!

Initial import of the package #2

Initial import of the package #2

Uh oh!

Conversation

nden commented Jun 16, 2020

Uh oh!

eslavich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eslavich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!