Refactor representations as a module by mhvk · Pull Request #14792 · astropy/astropy

mhvk · 2023-05-08T16:44:46Z

This is a reimplementation of #13688, redoing the refactoring to ensure that (1) there are no changes other than what is strictly necessary, and (2) any commits since that time are included (I restarted when I noticed #13688 had an import of a deprecated function that had been removed, and worried there were other changes). I avoided introducing any __all__ in the submodules, for now just relying on importing what is relevant in the new __init__.py. I cherry-picked commits from #13688 that fix errors in the documentation and in serialization because of the class name changes (making a few small changes in the process). Like in #13688, I also keep a few variables in __init__.py that are not exported, but are used in tests, for backward compatibility.

I mostly did this since I still think the refactoring is a very good idea, and it would be helpful for #14763, which introduces more general geodetic representations, if there is a place to put them.

Ping @nstarman who did #13688 and @eerovaher who had requested changes to it, mostly to ensure we make no actual code changes (which I think I ensured!); if we all still think this is a good idea, we should probably get it in sooner rather than later... (thankfully, representations is quite mature, so I don't foresee problems with backporting to 5.3).

p.s. The commits are meant to be fairly logical: for all pytest -P coordinates worked (except a few names in doctests after spherical is factored out).

EDIT: Close #13688

Only change is to use absolute imports for other modules in astropy.coordinates.

Just copied text over to new file and added required imports on top. In __init__.py, added import from new .cartesian at bottom to avoid circular imports.

Again, just adding imports on top, and an import from cylindrical at the bottom of __init__.py.

With usual local imports on top, and import from spherical at bottom of __init__.py. Also change to import in cyclindrical, and adjustment of one test, since the location of the classes now has changed. Not yet changing the locations in the failing doctests.

Now __init__.py can be cleaned up. Import some variables for backward compatibility. For the one private one only used in a test, change the test. Change imports on top of cartesian, etc., to be from .base.

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

github-actions · 2023-05-08T16:45:25Z

nstarman

This looks good and was very easy to review, stepping though the commits!

nstarman · 2023-05-08T17:49:26Z

+from .base import DIFFERENTIAL_CLASSES
+from .base import DUPLICATE_REPRESENTATIONS
+from .base import REPRESENTATION_CLASSES


Would it be worthwhile to make these read-only?

Suggested change

from .base import DIFFERENTIAL_CLASSES

from .base import DUPLICATE_REPRESENTATIONS

from .base import REPRESENTATION_CLASSES

from types import MappingProxyType

from .base import MappingProxyType(DIFFERENTIAL_CLASSES)

from .base import MappingProxyType(DUPLICATE_REPRESENTATIONS)

from .base import MappingProxyType(REPRESENTATION_CLASSES)

Or to deprecate their access from here?

I agree it should be deprecated, which is relatively easy to do with __getattr__, but think it is best to do as a separate PR after this, so one would not be distracted by all the other changes.

nstarman · 2023-05-08T17:53:25Z

+    "astropy.coordinates.representation.cartesian.CartesianRepresentation",
+    "astropy.coordinates.representation.spherical.UnitSphericalRepresentation",
+    "astropy.coordinates.representation.spherical.RadialRepresentation",
+    "astropy.coordinates.representation.spherical.SphericalRepresentation",
+    "astropy.coordinates.representation.spherical.PhysicsSphericalRepresentation",
+    "astropy.coordinates.representation.cylindrical.CylindricalRepresentation",
+    "astropy.coordinates.representation.cartesian.CartesianDifferential",
+    "astropy.coordinates.representation.spherical.UnitSphericalDifferential",
+    "astropy.coordinates.representation.spherical.SphericalDifferential",
+    "astropy.coordinates.representation.spherical.UnitSphericalCosLatDifferential",
+    "astropy.coordinates.representation.spherical.SphericalCosLatDifferential",
+    "astropy.coordinates.representation.spherical.RadialDifferential",
+    "astropy.coordinates.representation.spherical.PhysicsSphericalDifferential",
+    "astropy.coordinates.representation.cylindrical.CylindricalDifferential",


Is there a way to avoid changing the path? All the classes are imported to astropy.coordinates.representation.

In cosmology I use a mapping to map from the fully-qualified path to a preferred path, e.g. astropy.coordinates.representation.cartesian.CartesianRepresentation to astropy.coordinates.CartesianRepresentation

You already opened #13689 for that purpose - where you made the suggestion of using entrypoints, which seems just right for this.

mhvk · 2023-05-08T20:49:49Z

@nstarman - I think all your points are good ones - but also feel best for this PR is to make only the most minimal changes...

nstarman

LGTM. Thanks @mhvk !

mhvk · 2023-05-10T15:20:58Z

OK, will merge now!

pllim · 2023-05-10T15:36:20Z

+3,728 −3,641 😱

mhvk · 2023-05-10T16:21:50Z

Of course, mostly exactly the same lines. And a file with 3600 lines of code really is overly long!

mhvk and others added 7 commits May 8, 2023 16:46

Move representation.py to its own directory, as plain __init__.py

47b6f81

Only change is to use absolute imports for other modules in astropy.coordinates.

Factor out cartesian parts.

ce84563

Just copied text over to new file and added required imports on top. In __init__.py, added import from new .cartesian at bottom to avoid circular imports.

Factor out cylindrical.

d6ab172

Again, just adding imports on top, and an import from cylindrical at the bottom of __init__.py.

Factor out spherical

c856699

With usual local imports on top, and import from spherical at bottom of __init__.py. Also change to import in cyclindrical, and adjustment of one test, since the location of the classes now has changed. Not yet changing the locations in the failing doctests.

Factor out base.

4bc797a

Now __init__.py can be cleaned up. Import some variables for backward compatibility. For the one private one only used in a test, change the test. Change imports on top of cartesian, etc., to be from .base.

sphinx class locs

3b0ca92

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

fix registration of mixin classes

d43b8e2

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

mhvk added coordinates Refactoring no-changelog-entry-needed labels May 8, 2023

mhvk added this to the v6.0 milestone May 8, 2023

mhvk requested review from eerovaher and nstarman May 8, 2023 16:44

mhvk requested review from Cadair, astrofrog, larrybradley and taldcroft as code owners May 8, 2023 16:44

github-actions Bot added table visualization.wcsaxes labels May 8, 2023

mhvk mentioned this pull request May 8, 2023

refactor representations as module #13688

Closed

3 of 13 tasks

nstarman reviewed May 8, 2023

View reviewed changes

mhvk added no-changelog-entry-needed and removed no-changelog-entry-needed labels May 8, 2023

nstarman approved these changes May 10, 2023

View reviewed changes

mhvk merged commit eadcd73 into astropy:main May 10, 2023

mhvk deleted the representations-as-module branch May 10, 2023 15:21

mhvk mentioned this pull request May 10, 2023

Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. #14763

Merged

braingram mentioned this pull request May 23, 2023

Update RepresentationConverter for new class paths in astropy dev astropy/asdf-astropy#181

Merged

Navigation Menu

Repository navigation

Refactor representations as a module - #14792

Refactor representations as a module#14792
mhvk merged 7 commits into
astropy:mainfrom
mhvk:representations-as-module

mhvk commented May 8, 2023 •

edited by pllim

Loading

Uh oh!

github-actions Bot commented May 8, 2023

Uh oh!

nstarman left a comment

Uh oh!

nstarman May 8, 2023

Uh oh!

mhvk May 8, 2023

Uh oh!

nstarman May 8, 2023

Uh oh!

nstarman May 8, 2023

Uh oh!

mhvk May 8, 2023

Uh oh!

mhvk commented May 8, 2023

Uh oh!

nstarman left a comment

Uh oh!

mhvk commented May 10, 2023

Uh oh!

pllim commented May 10, 2023

Uh oh!

mhvk commented May 10, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-from .base import DIFFERENTIAL_CLASSES
-from .base import DUPLICATE_REPRESENTATIONS
-from .base import REPRESENTATION_CLASSES
+from types import MappingProxyType
+from .base import MappingProxyType(DIFFERENTIAL_CLASSES)
+from .base import MappingProxyType(DUPLICATE_REPRESENTATIONS)
+from .base import MappingProxyType(REPRESENTATION_CLASSES)

Conversation

mhvk commented May 8, 2023 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 8, 2023

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman May 8, 2023

Choose a reason for hiding this comment

Uh oh!

mhvk May 8, 2023

Choose a reason for hiding this comment

Uh oh!

nstarman May 8, 2023

Choose a reason for hiding this comment

Uh oh!

nstarman May 8, 2023

Choose a reason for hiding this comment

Uh oh!

mhvk May 8, 2023

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 8, 2023

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 10, 2023

Uh oh!

pllim commented May 10, 2023

Uh oh!

mhvk commented May 10, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhvk commented May 8, 2023 •

edited by pllim

Loading