refactor representations as module by nstarman · Pull Request #13688 · astropy/astropy

nstarman · 2022-09-16T16:52:02Z

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

It's a long file that does many things. Now it's one module with many files, each of which do fewer things.

~~For reviewers: I didn't change the test files, only moved them, so that should show that no (tested) functionality has changed!~~

TODO?

changelog, is one needed for an internal refactor?
break up the test files? Low priority, perhaps push to future PR.
~~BREAKING CHANGE: change folder name to representations~~

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

github-actions · 2022-09-16T16:52:50Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

nstarman · 2022-09-16T17:05:43Z

@astropy/testing-infrastructure-maintainers pytest_doctestplus is broken?

mhvk · 2022-09-16T17:16:24Z

Sorry pressed the "convert to real PR" in the wrong PR. Though I like the idea, so I guess we might as well.

mhvk

I like splitting up the module, but am less sure about also moving tests (partially since that means my memory of where test files lives will go haywire, while for representations itself, I'll tab-complete my way easily). I also think the tests themselves could do with a bit of reorganization, so I'd suggest separating the two steps. (An additional advantage is that then the test files will help ensure all from astropy.coordinates.reoresentation import ??? continue to work.)

I would also not split the various spherical representations - they are so clearly related that I think it is easier if they stay together (e.g., to keep the various quick transformations in one place), i.e., I would combine back radial, spherical, base_sphere, physicssphere and unitsphere, leaving as new modules base, cartesian, cylindrical, and spherical.

pllim · 2022-09-16T19:28:24Z

Re: doctestplus -- no idea, hard to see with +3,796 −3,523

nstarman · 2022-09-16T20:03:22Z

Interesting. De-serialization is an issue since the qualitative name has changed.

pllim · 2022-09-16T20:04:19Z

I just reran the last CI on main to see if the failure is caused by your changes or something else: https://github.com/astropy/astropy/actions/runs/3069379004

pllim · 2022-09-16T20:19:51Z

main still is green, so it is definitely you... 😅

taldcroft · 2022-09-16T22:31:08Z

About the test failure

        if cls_full_name not in __construct_mixin_classes:
>           raise ValueError(f'unsupported class for construct {cls_full_name}')
E           ValueError: unsupported class for construct astropy.coordinates.representation.cartesian.CartesianRepresentation

This is real assuming the class full name changed. There is a hard-coded lists of classes that the mixin constructor is allowed to construct, which is there for security reasons. It's hard-coded as strings so table doesn't have to import the rest of astropy always. @mhvk have talked about this before and never come up with a better way.

It's probably OK to just changed that hardcoded list, but doing stuff like this always makes you wonder if you will be breaking anything outside of astropy. [EDIT: now I see you did exactly that in 679db4c]

mhvk · 2022-09-16T22:46:18Z

Oops, I guess that also means that we have to continue to support both astropy.coordinates.representation.CartesianRepresentation as well as the same but with cartesian inserted. Unless we start to muck with the __module__ in this PR, but I've never liked if things do not point to where they are defined. Another option might be for __construct_mixin_classes to allow wildcards.

pllim · 2022-09-16T22:50:39Z

Maaaybe we need to hold off on this until we unpin Sphinx. The coordinate imports galore already broke compat with Sphinx. I don't want this to break it more.

nstarman · 2022-09-17T17:25:33Z

I opened #13689 to discuss improvements to __construct_mixin_classes.

@nstarman

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

@nstarman

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

@nstarman

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

@nstarman

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

nstarman · 2022-12-18T18:02:29Z

v5.2 is out, so this PR is ready to go in 🤞!

Stale

pllim · 2022-12-19T22:06:54Z

@mhvk requested changes in the review status, so maybe he wants to re-review?

And just to make sure, you did rebase recently enough to pick up the Sphinx unpinning, right?

nstarman · 2022-12-19T23:12:09Z

I believe so. That was some time last week, right?

eerovaher

Splitting up the representation classes into separate files is a good idea, but such a pull request that is moving lots of code around should try to avoid making any changes that are not necessary, at least not in the same commit that moves the code. @nstarman, it looks like you have some automatic tool (at least I hope it's an automatic tool) that edits comments and multi-line strings so that they fit into an 80 character line length limit. This does not affect the working of the code, but it is creating a lot of clutter in the diffs. Furthermore, the actual line length limit in astropy is now 88 characters. I've only marked two cases where the difference of enforced line lengths has led to needless editing of multi-line strings, but I expect there are more.

eerovaher · 2022-12-19T23:22:57Z

+                diff = differentials[key]
+            except TypeError as err:
+                raise TypeError(
+                    "'differentials' argument must be a " "dictionary-like object"


Why is this split into two strings? It's a single string in main:

astropy/astropy/coordinates/representation.py

Lines 745 to 747 in 2fab3d7

raise TypeError(

"'differentials' argument must be a dictionary-like object"

) from err

eerovaher · 2022-12-19T23:24:43Z

+                    "To attach a RadialDifferential to a "
+                    "UnitSphericalRepresentation, you must supply "
+                    "a dictionary with an appropriate key."


On main this error message is written in two lines, both within the 88 character line length limit, so there's no reason to split this into three lines here.

astropy/astropy/coordinates/representation.py

Lines 733 to 736 in 2fab3d7

raise ValueError(

"To attach a RadialDifferential to a UnitSphericalRepresentation,"

" you must supply a dictionary with an appropriate key."

)

nstarman · 2022-12-20T02:24:17Z

@nstarman, it looks like you have some automatic tool (at least I hope it's an automatic tool) that edits comments and multi-line strings so that they fit into an 80 character line length limit.

Yes, VSCode. I'll go thru the comments and compare. Revert where necessary. Or maybe change VSCode to have an 88 character limit and let it auto format... 🤔

@nstarman

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

@nstarman

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

nstarman · 2022-12-20T19:17:47Z

@eerovaher, I did the latter and then went and compressed error message strings into the minimum number of lines.

nstarman · 2022-12-19T15:18:56Z

+from .base import _invalidate_reprdiff_cls_hash  # noqa: F401
+from .base import get_reprdiff_cls_hash  # noqa: F401
+
+__all__ = [


Should this __all__ be empty so only the coordinates namespace explicitly holds these objects?

I'm with @eerovaher -- as few changes here as possible. So, let's keep this the same as the one originally in representations.py.

nstarman · 2022-12-20T19:17:57Z

+                    "To attach a RadialDifferential to a "
+                    "UnitSphericalRepresentation, you must supply "
+                    "a dictionary with an appropriate key."


nstarman · 2022-12-20T19:18:00Z

+                diff = differentials[key]
+            except TypeError as err:
+                raise TypeError(
+                    "'differentials' argument must be a " "dictionary-like object"


mhvk

Looks mostly OK, but I'm still unhappy with the empty __all__ -- readability counts, and this is done not for people reading the code, for whom it would be useful to know what classes are actually the ones being defined that get used elsewhere. Plus, that's how its done elsewhere in astropy.

mhvk · 2022-12-20T19:24:00Z

+from .base import _invalidate_reprdiff_cls_hash  # noqa: F401
+from .base import get_reprdiff_cls_hash  # noqa: F401
+
+__all__ = [


I'm with @eerovaher -- as few changes here as possible. So, let's keep this the same as the one originally in representations.py.

mhvk · 2022-12-20T19:27:58Z

+from astropy.utils.data_info import MixinInfo
+from astropy.utils.exceptions import DuplicateRepresentationWarning
+
+__all__: list[str] = []  # Not public. Import from ``astropy.coordinates``


I disagree with this still. Let's put things in here that get transferred up (like is done in modules in builtin_frames). If you feel strongly otherwise, I would suggest for now not having any __all__ (like in matrix_utilities, which is also not exposed). Though to me it is helpful to the code reader -- the ones for whom this matters most -- that it lists the classes, etc., that this file actually defines for use elsewhere.

mhvk · 2022-12-20T19:30:05Z

+
+def _fqn_class(cls: type) -> str:
+    """Get the fully qualified name of a class."""
+    return f"{cls.__module__}.{cls.__qualname__}"


Oddly, changed line.

mhvk · 2022-12-20T19:34:55Z

+
+from .base import BaseDifferential, BaseRepresentation
+
+__all__: list[str] = []  # Not public. Import from ``astropy.coordinates``


mhvk · 2022-12-20T19:36:54Z

@@ -0,0 +1,156 @@
+from __future__ import annotations


So, why is the license file still not added automatically then?

mhvk · 2022-12-20T19:38:33Z

@@ -0,0 +1,1600 @@
+from __future__ import annotations


I think there is no need for this if we remove __all__.

mhvk · 2022-12-20T19:40:19Z

+from .base import BaseDifferential, BaseRepresentation
+from .cartesian import CartesianRepresentation
+
+__all__: list[str] = []  # Not public. Import from ``astropy.coordinates``


mhvk · 2022-12-20T19:43:27Z


 The representation can be supplied either as a
-`~astropy.coordinates.representation.BaseRepresentation` class (e.g.,
+`~astropy.coordinates.representation.base.BaseRepresentation` class (e.g.,


I think this is better changed to ~astropy.coordinates.BaseRepresentation, since that is where we want users to import from.

eerovaher

I've highlighted a couple of needless changes to strings that are still present, but I have not made a thorough search even in the one file I looked at, so I expect there to be many more cases of this.

@nstarman, the tool that you are using now seems to respect the 88 character line length limit, but it is being very aggressive about it, i.e. it adds words from the beginning of the next line to the end of the current line if they fit. This is being done in multi-line strings, comments and docstrings.

I supported adopting black because I was annoyed with the number of commits in this repository that introduced formatting problems that had to be fixed in a separate commit. Such formatting fixes add clutter to the commit log and make it more difficult to track actual functional changes. black together with pre-commit can prevent such formatting-only commits from ever happening (although not everyone is making use of these tools). The tool you are using is making cosmetic changes to a codebase that black is happy with. This is adding clutter to the commit history, which I cannot approve. I recommend you turn off any code-formatting features VSCode might have and just let black do its job.

eerovaher · 2022-12-20T23:50:25Z

+
+        if not hasattr(cls, "attr_classes"):
+            raise NotImplementedError(
+                "Representations must have an " '"attr_classes" class attribute.'


This string is still getting needlessly split up. Here's what it looks like in current main:

astropy/astropy/coordinates/representation.py

Line 653 in 8796307

'Representations must have an "attr_classes" class attribute.'

eerovaher · 2022-12-20T23:55:48Z

+            msg = (
+                f'Representation "{repr_name}" already defined, removing it to avoid '
+                f'confusion. Use qualnames "{fqn_cls}" and "{fqn_existing}" or class '
+                "instances directly"
+            )


Here the placement of spaces separating the sub-strings is being needlessly changed. Currently on main the spaces are put in the beginning of the sub-strings:

astropy/astropy/coordinates/representation.py

Lines 669 to 673 in 8796307

msg = (

f'Representation "{repr_name}" already defined, removing it to avoid'

f' confusion.Use qualnames "{fqn_cls}" and "{fqn_existing}" or class'

" instances directly"

)

eerovaher · 2022-12-21T00:01:19Z

+    def __setitem__(self, item, value):
+        if not isinstance(value, BaseRepresentation):
+            raise TypeError(
+                f"value must be a representation instance, " f"not {type(value)}."


One f-string is good enough for main, it should be good enough here as well.

astropy/astropy/coordinates/representation.py

Line 1070 in 8796307

f"value must be a representation instance, not {type(value)}."

pllim · 2023-04-18T13:34:26Z

Wow +3,728 −3,658. I think this is too late to squeeze into v5.3 only 3 days before feature freeze. I am re-milestoning this.

I would recommend you wrap this up early in the v6.0 dev cycle. Or maybe breaking it into a series of smaller PRs.

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

See #14792 for a re-implementation of this - I still think this is an excellent idea but this PR was getting behind main and there were still a few additions that were a little controversial. I tried to keep it as much of a copy & paste effort as possible.

nstarman added this to the v5.2 milestone Sep 16, 2022

nstarman requested a review from mhvk September 16, 2022 16:52

github-actions Bot added coordinates io.misc table utils labels Sep 16, 2022

nstarman force-pushed the coords-rep-module branch from de2fc3e to c2e77a5 Compare September 16, 2022 16:57

mhvk marked this pull request as ready for review September 16, 2022 17:15

mhvk requested review from WilliamJamieson, matteobachetti and taldcroft as code owners September 16, 2022 17:15

mhvk requested changes Sep 16, 2022

View reviewed changes

pllim reviewed Sep 16, 2022

View reviewed changes

Comment thread astropy/table/tests/test_mixin.py Resolved

nstarman force-pushed the coords-rep-module branch from c2e77a5 to 06b1675 Compare September 16, 2022 19:48

nstarman force-pushed the coords-rep-module branch from 06b1675 to a380c95 Compare September 16, 2022 20:04

nstarman force-pushed the coords-rep-module branch 2 times, most recently from d24f7f6 to 679db4c Compare September 16, 2022 22:04

nstarman requested review from Cadair and astrofrog as code owners September 16, 2022 22:04

nstarman force-pushed the coords-rep-module branch 2 times, most recently from aa9f0da to 7e21a10 Compare November 19, 2022 22:11

nstarman added 4 commits December 17, 2022 19:18

refactor as module

c88d572

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

Add necessary imports

3db1266

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

fix registration of mixin classes

7af72a6

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

sphinx class locs

d379868

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

nstarman force-pushed the coords-rep-module branch from 7e21a10 to d379868 Compare December 18, 2022 00:31

nstarman requested review from eerovaher, mhvk and pllim December 18, 2022 00:32

eerovaher reviewed Dec 19, 2022

View reviewed changes

nstarman added 2 commits December 20, 2022 14:03

fix line length wraps

7cf4895

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

compress error messages

dc78a15

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

nstarman commented Dec 20, 2022

View reviewed changes

mhvk reviewed Dec 20, 2022

View reviewed changes

eerovaher reviewed Dec 21, 2022

View reviewed changes

larrybradley removed their request for review February 3, 2023 19:03

pllim modified the milestones: v5.3, v6.0 Apr 18, 2023

mhvk mentioned this pull request Apr 30, 2023

Refactor EarthLocation as instance of BodyLocation. Add flattening and equatorial radius as parameters. #14714

Closed

mhvk mentioned this pull request May 8, 2023

Refactor representations as a module #14792

Merged

mhvk closed this in #14792 May 10, 2023

	raise TypeError(
	"'differentials' argument must be a dictionary-like object"
	) from err

	raise ValueError(
	"To attach a RadialDifferential to a UnitSphericalRepresentation,"
	" you must supply a dictionary with an appropriate key."
	)


		from .base import BaseDifferential, BaseRepresentation

		__all__: list[str] = [] # Not public. Import from ``astropy.coordinates``

	msg = (
	f'Representation "{repr_name}" already defined, removing it to avoid'
	f' confusion.Use qualnames "{fqn_cls}" and "{fqn_existing}" or class'
	" instances directly"
	)

Conversation

nstarman commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for package maintainer(s)

Uh oh!

github-actions Bot commented Sep 16, 2022

Uh oh!

nstarman commented Sep 16, 2022

Uh oh!

mhvk commented Sep 16, 2022

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pllim commented Sep 16, 2022

Uh oh!

Uh oh!

nstarman commented Sep 16, 2022

Uh oh!

pllim commented Sep 16, 2022

Uh oh!

pllim commented Sep 16, 2022

Uh oh!

taldcroft commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Sep 16, 2022

Uh oh!

pllim commented Sep 16, 2022

Uh oh!

nstarman commented Sep 17, 2022

Uh oh!

nstarman commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Dec 19, 2022

Uh oh!

nstarman commented Dec 19, 2022

Uh oh!

eerovaher 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!

nstarman commented Dec 20, 2022

Uh oh!

nstarman commented Dec 20, 2022

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!

mhvk 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!

nstarman commented Sep 16, 2022 •

edited

Loading

taldcroft commented Sep 16, 2022 •

edited

Loading

nstarman commented Dec 18, 2022 •

edited

Loading

eerovaher Dec 20, 2022 •

edited

Loading