Galactocentric frame defaults registry by nstarman · Pull Request #10624 · astropy/astropy

nstarman · 2020-08-04T21:46:43Z

Description

galactocentric_frame_defaults allows for a user-defined set of parameters to be manually set. This PR extends that capability to a registry system such that parameter sets may be named and get/set just like the built-in option (pre-v4.0, v4.0).

import astropy.units as u
from astropy.coordinates import galactocentric_frame_defaults

galactocentric_frame_defaults.register(
    name="test",
    value={"galcen_coord": ICRS(ra=266.0 * u.degree, dec=-29 * u.degree),
                "galcen_distance": 8.1 * u.kpc,
                "galcen_v_sun": r.CartesianDifferential([12.9, 245.6, 7.8] * u.km / u.s),
                "z_sun": 21.0 * u.pc,
                 "roll": 0 * u.deg},
    references=None
)

with galactocentric_frame_defaults.set("test"):
    params = galactocentric_frame_defaults.get()

Additional metadata, such as the value standard deviations, is supported by the _state dictionary attribute, whose items are also exposed to the user by key access on the main class.

galactocentric_frame_defaults._registry["test"]["stdv"] = {
    "galcen_coord": {"ra": 1 * u.deg, "dec": 1 * u.deg},
    "galcen_distance": 0.1 * u.kpc,
    "galcen_v_sun": {"x": 2 * u.km / u.s, "y": 1 * u.km / u.s, "z": 1* u.km / u.s},
    "z_sun": 1 * u.pc,
    "roll": 0 * u.deg,
}

galactocentric_frame_defaults.set("test")

test_stdv = galactocentric_frame_defaults["stdv"]

If metadata is added to one parameter set, for consistency it should be added to all. Though the built-in options are read-only for "value" and "references", they permit additional fields, such as the standard deviation.

galactocentric_frame_defaults._registry["v4.0"]["stdv"] = {
    "galcen_coord": {"ra": 1 * u.deg, "dec": 1 * u.deg},
    "galcen_distance": 0.1 * u.kpc,
    "galcen_v_sun": {"x": 2 * u.km / u.s, "y": 1 * u.km / u.s, "z": 1* u.km / u.s},
    "z_sun": 1 * u.pc,
    "roll": 0 * u.deg,
}

galactocentric_frame_defaults.set("v4.0")

stdv = galactocentric_frame_defaults["stdv"]

pep8speaks · 2020-08-04T22:36:33Z

Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-09-16 20:03:53 UTC

nstarman · 2020-08-05T15:32:15Z

If this PR aligns with Astropy's goals for ScienceState, I think there's room to push further. In particular, the _value attribute could be replaced by the _state attribute, which allows for more flexibility. Backward compatibility and attribute-style access to _value can be maintained by the ScienceState metaclass as @property, or perhaps, something like the BaseCoordinateFrame method of injecting attributes. Also, the _Context returned by set could store the old state and redirect all getitem to self._parent. This would allow the __enter__ method to return the context for use in with statements.

MyState._value = 1
with MyState.set(value=2) as context:
    assert context._value == MyState._value == 2
assert MyState._value == 1
with pytest.raises(Exception):
    context._value

This currently does not work as the context would have _value=1 in with.
This PR already implements most of the underlying mechanics to make this possible, including clearing the context __dict__ on __exit__.

adrn · 2020-08-05T18:07:59Z

Cool, thanks for this @nstarman! I'll review this sometime over the next week or so.

mhvk

I like the new registry very much! There are some in-line comments, mostly to help clarify.

My main suggestion would be to not add the StateMixinMeta metaclass, at least not in this PR. The ScienceState class is used in quite a few other places, and I think we should first make sure it provides greatest benefit to all (and that it doesn't make expanding them more difficult). I also think the benefit here is small at best - I don't really see why the ScienceState classes should have dict-like behaviour. My overall sense would be just to keep things minimal, but I could see instead adding a few @classproperty to help get more information on the current state (or perhaps expand what .get() can return).

nstarman · 2020-08-07T19:05:17Z

The question is which to do first?

Based upon the comments, particularly the implementation details to make a __getattr__ work over a __getitem__, I agree that the metaclass and most modifications to ScienceState are for a different PR.

My main suggestion would be to not add the StateMixinMeta metaclass, at least not in this PR. The ScienceState class is used in quite a few other places, and I think we should first make sure it provides greatest benefit to all (and that it doesn't make expanding them more difficult). I also think the benefit here is small at best - I don't really see why the ScienceState classes should have dict-like behaviour.
My overall sense would be just to keep things minimal, but I could see instead adding a few @classproperty to help get more information on the current state (or perhaps expand what .get() can return).

I do have a fix for all this, but I think should be the subject of a different PR since it is a more substantial change to ScienceState. The changes to galactocentric_frame_defaults would be minimal. The question is which to do first? pushing a minimal change here that adds the registry will need to be changed a little again if the following PR is accepted.

As a preview for a ScienceState PR. It is a drop-in replacement for all the current ScienceState classes. It should allow the solar_system_ephemeris ScienceState to automatically implement the correct kernel, for which the class currently has special fixes. the state has a central value (value) but any other related state values are accessible as attributes. The Context class can be used in with statements and correctly references the current set state, etc. While these features are nice, the proposed are pretty substantial changes to the inner workings of ScienceState and affect multiple modules.

galactocentric.py.txt
state.py.txt

(Unfortunately these had to be attached as .txt)

mhvk · 2020-08-07T19:24:23Z

@nstarman - looks like we agree - let's just split this into a first part that adds the registration and a follow-up PR that improves ScienceState. I do not think it is a problem at all to make a further small change to the galactocentric_frame_defaults class in that second PR. And I like the idea of simplifying the solar system one - the implementation Erik, Stuart and I made always felt a bit like a hack!

nstarman · 2020-08-11T16:55:26Z

With the exception of moving _Context out of ScienceState.set this PR is now just the galactocentric registry.

mhvk

To me, this looks very nice and my comments are mostly nitpicks. I would very much like @adrn to look at this too.

adrn · 2020-08-12T12:50:10Z

Mainly minor comments from me, but otherwise this is looking great!

adrn

I'm happy with this, apart from a few remaining minor comments!

nstarman

Ready for final review?

mhvk · 2020-08-24T16:11:34Z

@nstarman - just to let you know this hasn't been forgotten! I hope to get to it soonish, if @adrn doesn't get to it before.

adrn

Sorry for the delay (just back from vacation). This is looking great! I have a few minor suggested changes, otherwise I think this is ready.

nstarman · 2020-08-31T14:58:21Z

Thanks for the comments @adrn . I think I have addressed / committed them all.
Is there an astropy-wide RTD failure happening? I'm getting an error that does not seem related to my edits.

ModuleNotFoundError: No module named 'setuptools._distutils'

And same with travis. Unrelated failures.

adrn

Great, thanks @nstarman! I think this is ready to go in, but I will give @mhvk an opportunity to have a last look. The RTD failure indeed looks unrelated.

mhvk

@nstarman - yes, almost there: definitely mostly nitpicks left...

Also, could you squash the commits while you make the last changes? (I think to just one, but if you feel that in-between steps are useful, maybe a few).

nstarman · 2020-08-31T19:56:06Z

Tried to make it squash to 1, but there was a conflict on force push, so the minimum appears to be 2.

RTD failure still unrelated.
Travis failure also appears unrelated. Something about a remote server.

mhvk

Always seem to see yet more when I look at code again. So sorry!

Anyway, it allows me to note that the way to get one commit (and avoid a merge commit in your branch, which we try to avoid [if mostly for backporting which is not relevant here]) is to do a rebase on master before git push -f.

See https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#how-to-rebase

mhvk · 2020-08-31T20:04:44Z

+        return copy.deepcopy(self)
+
+
+def _reduce_stateproxy(sp: _StateProxy):


I think the advantage of the class above is that it can now define __getstate__ instead of registering this override. Or am I missing something? I am actually slightly confused why this needed - why not adjust the .copy method itself?

Unfortunately, a __getstate__ method doesn't work in this case. I could create a custom __deepcopy__ method, but the copyreg method also allows the object to be pickled.

https://docs.python.org/3/library/copy.html
ultimate and penultimate paragraphs

But with this code, doesn't it mean that if you unpickle the pickled instance, you get a dict rather than a _StateProxy?

I'm actually not quite sure whether it is possible to pickle and unpickle the class itself, but it would not seem good that if I pickled the registry for use in another process, all the registries would become dict... Especially if the goal is to ensure to avoid a place being made to propagate back.

Overall, I do wonder if we're not over-engineering this. Might it not make more sense to just create a new dict (i.e., do the deep copy) inside the get_registry method? (I vaguely fear this is returning to a previous design -- if so, sorry!).

If I switch from copyreg to implementing __deepcopy__ in _StateProxy, then the deep copy will happen inside the get_from_registry method and it will have identical behavior to MappinProxy (ie read-only), but also implement getitem. It will not be able to be pickled, so should not have any unexpected behavior.

mhvk · 2020-08-31T21:48:14Z

Out of the code thread, maybe good to explicitly discuss (cc @adrn): the idea of the MappingProxy here is to guard against people naively getting the state and just changing it, which would change the state internals, and might well lead to very inconsistent result.

I can see the benefit, but am wondering whether it is worth the complexity and performance, given the extra copies any time the state is queried. I also worry about giving a false sense of security, if not for this code, then for the rest of astropy!

p.s. Don't want to make a big deal of this, though! Just thinking the extra code now introduced to make the MappingProxy work is quite large, making it harder for a new person seeing the code to understand what is going on - this very much in contrast to the rest, which I think makes the state quite a bit clearer!

nstarman · 2020-08-31T22:13:47Z

Anyway, it allows me to note that the way to get one commit (and avoid a merge commit in your branch, which we try to avoid [if mostly for backporting which is not relevant here]) is to do a rebase on master before git push -f.

Thanks for the tip.

nstarman · 2020-08-31T22:42:41Z

Out of the code thread, maybe good to explicitly discuss (cc @adrn): the idea of the MappingProxy here is to guard against people naively getting the state and just changing it, which would change the state internals, and might well lead to very inconsistent result.

I can see the benefit, but am wondering whether it is worth the complexity and performance, given the extra copies any time the state is queried. I also worry about giving a false sense of security, if not for this code, then for the rest of astropy!

p.s. Don't want to make a big deal of this, though! Just thinking the extra code now introduced to make the MappingProxy work is quite large, making it harder for a new person seeing the code to understand what is going on - this very much in contrast to the rest, which I think makes the state quite a bit clearer!

I think the new implementation is a lot cleaner. Instead of direct MappingProxyType, the read-only copies are maintained by a MappingView subclass — StateProxy -- that adds a getitem and custom __deepcopy__. The standard MappingView deepcopy returns a MappingView, StateProxy returns a dict. No more copyreg. MappingProxyType is the optimal solution when no deepcopy is needed, but that isn't the case here.

mhvk

two lines that are no longer needed left...
Otherwise, looks great!

nstarman · 2020-09-01T00:51:37Z

@adrn @mhvk
Thanks for all the help getting this ready. I had all this custom code that was just accumulating. This gave me a taste of the PR life and now I've got 4 more and a couple issues going.

@mhvk

@mhvk astropy#10624 (comment) Signed-off-by: Nathaniel Starkman <[email protected]>

@mhvk

@mhvk astropy#10624 (comment) Signed-off-by: Nathaniel Starkman <[email protected]>

mhvk · 2020-09-15T18:58:39Z

Darn, was just looking at this to merge it and now it has a conflict... Could you rebase/sqash one more time, @nstarman? Thanks!

Signed-off-by: Nathaniel Starkman <[email protected]>

nstarman · 2020-09-16T20:38:06Z

Could you rebase/sqash one more time

@mhvk, done. As far as I can tell, the failures are unrelated.

mhvk · 2020-09-16T20:47:11Z

Thanks, @nstarman!

adrn · 2020-09-16T20:47:27Z

🎉 A huge thanks to you @nstarman for this!

(CI failures were indeed unrelated.)

nstarman · 2020-09-16T20:48:45Z

Thanks for all the comments and suggestions! This was a great learning experience.

astropy-bot Bot added coordinates utils labels Aug 4, 2020

pllim added the Enhancement label Aug 5, 2020

pllim added this to the v4.2 milestone Aug 5, 2020

pllim requested review from adrn and eteq August 5, 2020 14:52

mhvk reviewed Aug 6, 2020

View reviewed changes

mhvk reviewed Aug 11, 2020

View reviewed changes

adrn reviewed Aug 12, 2020

View reviewed changes

nstarman requested review from adrn and mhvk August 17, 2020 21:13

adrn reviewed Aug 18, 2020

View reviewed changes

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

Comment thread docs/coordinates/galactocentric.rst Resolved

nstarman commented Aug 21, 2020

View reviewed changes

adrn reviewed Aug 31, 2020

View reviewed changes

adrn approved these changes Aug 31, 2020

View reviewed changes

adrn added the Ready-for-final-review label Aug 31, 2020

mhvk reviewed Aug 31, 2020

View reviewed changes

nstarman force-pushed the galactocentric_frame_defaults_registry branch from 58e6c7e to c3b0924 Compare August 31, 2020 19:38

mhvk reviewed Aug 31, 2020

View reviewed changes

nstarman force-pushed the galactocentric_frame_defaults_registry branch from 4f33c95 to f07fbeb Compare August 31, 2020 22:13

nstarman force-pushed the galactocentric_frame_defaults_registry branch 2 times, most recently from f6a7cdc to 133667d Compare August 31, 2020 22:54

mhvk reviewed Sep 1, 2020

View reviewed changes

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

Comment thread astropy/coordinates/builtin_frames/galactocentric.py Resolved

nstarman force-pushed the galactocentric_frame_defaults_registry branch from 133667d to 9b9d46c Compare September 1, 2020 00:46

nstarman added a commit to nstarman/astropy that referenced this pull request Sep 2, 2020

move imports to top of file in test_frames

77245cf

@mhvk astropy#10624 (comment) Signed-off-by: Nathaniel Starkman <[email protected]>

This was referenced Sep 2, 2020

move imports to top of file in test_frames #10703

Merged

Meta PR: Extensible Cosmology #10673

Closed

migueldvb pushed a commit to migueldvb/astropy that referenced this pull request Sep 7, 2020

move imports to top of file in test_frames

a33244c

@mhvk astropy#10624 (comment) Signed-off-by: Nathaniel Starkman <[email protected]>

nstarman force-pushed the galactocentric_frame_defaults_registry branch from 4db4100 to 1fd3d76 Compare September 11, 2020 23:23

Add registry to galactocentric_frame_defaults

d54a9a3

Signed-off-by: Nathaniel Starkman <[email protected]>

nstarman force-pushed the galactocentric_frame_defaults_registry branch from 1fd3d76 to d54a9a3 Compare September 16, 2020 20:03

adrn approved these changes Sep 16, 2020

View reviewed changes

adrn merged commit ccffc99 into astropy:master Sep 16, 2020

nstarman mentioned this pull request Sep 20, 2020

ScienceState states #10751

Closed

nstarman deleted the galactocentric_frame_defaults_registry branch September 22, 2020 05:08

pllim mentioned this pull request Aug 18, 2021

Cannot use older versions of constants #12082

Closed

		return copy.deepcopy(self)


		def _reduce_stateproxy(sp: _StateProxy):

Conversation

nstarman commented Aug 4, 2020

Description

Uh oh!

pep8speaks commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-09-16 20:03:53 UTC

Uh oh!

nstarman commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrn commented Aug 5, 2020

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!

Uh oh!

Uh oh!

Uh oh!

nstarman commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Aug 7, 2020

Uh oh!

nstarman commented Aug 11, 2020

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrn commented Aug 12, 2020

Uh oh!

adrn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 24, 2020

Uh oh!

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

Uh oh!

nstarman commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Aug 4, 2020 •

edited

Loading

nstarman commented Aug 5, 2020 •

edited

Loading

nstarman commented Aug 7, 2020 •

edited

Loading

nstarman left a comment •

edited

Loading

nstarman commented Aug 31, 2020 •

edited

Loading

nstarman commented Aug 31, 2020 •

edited

Loading

nstarman commented Aug 31, 2020 •

edited

Loading

nstarman commented Sep 1, 2020 •

edited

Loading