Factor out class generator mechanics by nstarman · Pull Request #12921 · astropy/astropy

nstarman · 2022-03-04T20:47:06Z

The class generator is factored out of Masked and Distribution as a meta class.

todo:

docstrings
documentation ?
tests? basically covered by Masked and Distribution
changelog entry

There's some residual weirdness with __init_subclass__ and a metaclasses that I haven't solved. Solutions / input appreciated.

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.

pep8speaks · 2022-03-04T20:47:11Z

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

In the file astropy/coordinates/builtin_frames/skyoffset.py:

Line 80:68: E999 SyntaxError: invalid syntax

In the file astropy/uncertainty/core.py:

Line 25:67: E999 SyntaxError: invalid syntax

In the file astropy/utils/masked/core.py:

Line 50:67: E999 SyntaxError: invalid syntax

In the file astropy/utils/metaclasses.py:

Line 207:50: E999 SyntaxError: invalid syntax

In the file astropy/utils/tests/test_metaclasses.py:

Line 20:49: E999 SyntaxError: invalid syntax

Comment last updated at 2022-06-25 20:55:56 UTC

github-actions · 2022-03-04T20:47:52Z

👋 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?

mhvk

This looks really nice! I was a bit doubtful at first when I saw the approach with the metaclass, but it is clear it makes a lot of sense, with __init__ and __call__ both very logically used.

I have not had a chance yet to see what goes wrong...

nstarman · 2022-03-05T20:09:19Z

~~I'm getting a very weird error that I can reproduce on pytest / tex, but not in Jupyter, where the example correctly finds the MRO and makes a MaskedNDArray.~~

Solved!

nstarman · 2022-03-23T17:08:52Z

@adrn, this might be used to solve JAX/Dask/etc + units.

Quantity becomes like Distribution or Masked and what people actually use is NumPyQuantity or DaskQuantity where the class is a multiple inheritance of Quantity and the array class.
One of the challenges will be re-implementing the __array_function__ overrides in a more array-class agnostic way.

nstarman · 2022-03-31T22:02:13Z

The RTD error is real, but all I've done is import abc.ABCMeta and use it as a base class. Hm....

nstarman · 2022-03-31T23:11:05Z

@pllim I don't think putting the wrapper meta class in utils/collections.py is the best place, but there isn't obviously a better one. Should we make a new file utils/metaclasses.py or something?

pllim · 2022-03-31T23:38:18Z

utils/metaclasses.py

I guess it doesn't hurt...

mhvk · 2022-04-01T13:11:04Z

What about astropy/utils/codegen.py? That seems fairly appropriate since we are creating new classes!

pllim · 2022-04-01T13:38:08Z

I think codegen is too cryptic... We don't want people to think it has AI code or something... 😸

nstarman · 2022-04-01T15:03:02Z

~~Oops. I can move it from codegen.~~

I called the new file abcs.py. I didn't want to call it abc.py, like the builtin, so the plural seemed fine.

nstarman · 2022-04-01T16:21:04Z

@mhvk @adrn @pllim. What do you think the next steps are? Do we need a set of tests for this beyond the tests in Masked and Distribution? More documentation? I don't think there should be anything yet in the narrative docs. Perhaps when this is used for Quantity then we'll write something up.

@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

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]>

mhvk

This is looking very nice! It seems clear to me that having subclasses of FactoryMeta is the right solution.

With that, one now can get to bikeshedding about names... My general sense would be that the metaclass names do not have to have the _inmix prefix, and in principle could even be public.

One a bit larger thing is that I worry at times a little too much moved to the metaclasses. I also have a sense some things could be simpler (the weak dicts being a particular example). But I really need a bit more time to look through this properly! As I'm off tomorrow, I'm not quite sure when I'll have time, but I'll try! So, I'll send the comments I have so far (somewhat random, I fear).

mhvk · 2022-06-26T01:01:16Z


-    def __init_subclass__(cls, base_cls=None, data_cls=None, **kwargs):
-        """Register a Masked subclass.
+    def from_unmasked(


I feel this should remain as a class method on Masked proper - after all, it was a public method, and currently it can be used from a masked instance.

It's reasonable to not want to change the API. I'll move it back.

mhvk · 2022-06-26T01:03:07Z

            mask = getattr(self.mask, method)(*args, **kwargs)

-        result = self.from_unmasked(data, mask, copy=False)
+        result = type(self).from_unmasked(data, mask, copy=False)


Keeping from_unmasked as a class method, this change would not be necessary.

mhvk · 2022-06-26T01:10:15Z

+class FactoryMeta(ABCMeta):
+    """Meta-class for generating classes of a data class.
+
+    Classes derived from this metaclass.


Incomplete sentence

mhvk · 2022-06-26T01:12:03Z

+        name: str,
+        bases: tuple[type, ...],
+        namespace: dict[str, Any],
+        base_cls: type | None=None,


Maybe add a * so base_cls and data_cls are keyword only (works by default, of course, but maybe clearer?)

mhvk · 2022-06-26T01:27:12Z

+
+        # inject data_cls into namespace before `__new__` so
+        # `__init_subclass__` can use it.
+        namespace["__intomixclass__"] = data_cls


Doesn't __init_subclass__ get whatever is passed in to the class anyway?

mhvk · 2022-06-27T15:47:28Z

+"""Astropy's ABCs."""
+
+from __future__ import annotations
+from ast import Raise


Opening this file in emacs showed wiggly red lines: Raise is not used.

Ah. yes. One of the less desirable side effects of VSCode auto-imports. I must have typed a Raise with capital and it tried to help. I try to pair VSCode with flake's detect-unused-imports in pre-commit to catch these. But we don't have that enabled... 🤔

mhvk · 2022-06-27T15:47:40Z

+import sys
+import weakref
+from abc import ABCMeta
+from typing import Sequence, Dict, Any, Tuple, TypeVar, Union


Union is not used.

mhvk · 2022-06-27T16:13:03Z

+        namespace: dict[str, Any],
+        base_cls: type | None=None,
+        data_cls: type | None=None,
+        **_: Any


Why not **kwargs and actually pass them on? In case someone inherits and puts something in between.

mhvk · 2022-06-27T16:14:48Z

+            self.__inmixbase__ = self
+            """Base class for generating new intermixed classes."""
+
+            self.__inmixed_base_classes = weakref.WeakKeyDictionary()


Using WeakKeyDictionary here seems to add a lot of complication for what seems very little gain: generally, if, say, a Masked subclass is created, it will happen again soon enough. Similarly for SkyOffsetFrame and Distribution. Plus, these dictionaries cannot get very big. I'd suggest to just use dict

mhvk · 2022-06-27T16:21:41Z


-    For subclasses of these unmasked classes, masked counterparts can be generated.
-    """
+        masked_cls = self._inmix_get_subclass(data.__class__)


Just

return super()._inmix_make_instance(data, mask, copy=copy)

Alternatively, keep it as a class method _get_masked_instance, which can call this directly. Or, perhaps even better, in that case just call cls(data, mask, copy=copy), which I think will do the right thing.

github-actions · 2022-11-23T05:35:48Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions · 2022-12-24T05:34:58Z

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

mhvk · 2023-04-26T06:21:35Z

@nstarman - working on Distribution a bit, I'm realizing that it could use some of the things Masked had, like new class generation with Distribution(class) - easiest would be through this PR. Do you want to revive it?

nstarman · 2023-05-01T14:53:51Z

I am interested in doing that. One major drawback I found is that we are missing an underlying piece of the mechanics: a resolution order of the class generation.

Let's say we make class InterpolatedCoordinate that adds an affine parameter to a Coordinate so that it describes a path. Then InterpolatedCoordinate(SkyOffsetFrame(...)) needs to understand how to reconstruct the dynamic class from the original type. I think it's the same as the MRO. Unifying the class generation framework is definitely better than the alternative, but it has some tricky subtleties.

mhvk · 2023-05-07T17:38:00Z

Yes, indeed, it is all a bit tricky. For Distribution, I have been finding that it is almost impossible to do it with just a single class - it seems one needs something on top (mostly for __getitem__ and __setitem__, perhaps also __new__) and something just above ndarray). So, maybe it is best to keep the default implementation really simple and just allow subclass factories to override.

p.s. It may also be that SkyOffsetFrame just doesn't quite fit; need to look at the code again!

nstarman · 2023-05-17T16:54:04Z

Not that this isn't useful, I just think a more studied approach is needed.

nstarman added this to the v5.1 milestone Mar 4, 2022

nstarman requested a review from mhvk March 4, 2022 20:47

github-actions Bot added uncertainty utils utils.masked labels Mar 4, 2022

mhvk reviewed Mar 5, 2022

View reviewed changes

Comment thread astropy/utils/masked/core.py Resolved

Comment thread astropy/utils/collections.py Resolved

Comment thread astropy/utils/masked/core.py Resolved

Comment thread docs/utils/masked/index.rst Resolved

Comment thread astropy/uncertainty/core.py Resolved

nstarman force-pushed the subclass-factor-out branch 2 times, most recently from 8a4a1b4 to 683fa13 Compare March 5, 2022 19:51

nstarman force-pushed the subclass-factor-out branch from 683fa13 to 9a0b438 Compare March 9, 2022 16:23

nstarman added the no-changelog-entry-needed label Mar 9, 2022

nstarman marked this pull request as ready for review March 9, 2022 16:55

nstarman requested a review from mhvk March 9, 2022 16:56

nstarman requested a review from adrn March 23, 2022 16:42

nstarman force-pushed the subclass-factor-out branch 3 times, most recently from a04da84 to ebd08a5 Compare March 31, 2022 21:19

nstarman removed the no-changelog-entry-needed label Mar 31, 2022

nstarman force-pushed the subclass-factor-out branch from 4f8e40b to 63a28de Compare April 1, 2022 16:17

pllim reviewed Apr 1, 2022

View reviewed changes

Comment thread astropy/utils/abcs.py Resolved

nstarman added 8 commits June 25, 2022 16:54

remove on_metaclass

f99f32d

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

Rename metaclass

d7eea84

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

rename everything

57232e8

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

add tests

f23f6b6

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

fix annotations

8cb406e

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

skyoffsetframe

fa1bc20

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

Rename to FactoryMeta

97fdb87

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

Address comments

be3f9e6

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

nstarman force-pushed the subclass-factor-out branch from 1f8c895 to be3f9e6 Compare June 25, 2022 20:55

mhvk reviewed Jun 27, 2022

View reviewed changes

nstarman mentioned this pull request Jul 11, 2022

Experiment with interfacing with Dask #8227

Open

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Nov 23, 2022

astrofrog modified the milestones: v5.2, v5.3 Dec 8, 2022

github-actions Bot added the closed-by-bot Closed by stale bot label Dec 24, 2022

github-actions Bot closed this Dec 24, 2022

nstarman added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Dec 31, 2022

nstarman reopened this Dec 31, 2022

nstarman removed the closed-by-bot Closed by stale bot label Jan 14, 2023

saimn modified the milestones: v5.3, v6.0 Apr 21, 2023

nstarman closed this May 17, 2023

wtbarnes mentioned this pull request Jul 15, 2023

Cannot create a Quantity with the new numpy.array_api #15071

Closed

Conversation

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

Uh oh!

Checklist for package maintainer(s)

Uh oh!

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

Uh oh!

Comment last updated at 2022-06-25 20:55:56 UTC

Uh oh!

github-actions Bot commented Mar 4, 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!

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

Uh oh!

Uh oh!

nstarman commented Mar 23, 2022

Uh oh!

nstarman commented Mar 31, 2022

Uh oh!

nstarman commented Mar 31, 2022

Uh oh!

pllim commented Mar 31, 2022

Uh oh!

mhvk commented Apr 1, 2022

Uh oh!

pllim commented Apr 1, 2022

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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!

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!

github-actions Bot commented Nov 23, 2022

Uh oh!

github-actions Bot commented Dec 24, 2022

Uh oh!

mhvk commented Apr 26, 2023

Uh oh!

nstarman commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented May 7, 2023

Uh oh!

nstarman commented May 17, 2023

nstarman commented Mar 4, 2022 •

edited

Loading

pep8speaks commented Mar 4, 2022 •

edited

Loading

nstarman commented Mar 5, 2022 •

edited

Loading

nstarman commented Apr 1, 2022 •

edited

Loading

nstarman commented Apr 1, 2022 •

edited

Loading

nstarman commented May 1, 2023 •

edited

Loading