astropy / astropy-v5.0-paper

Astropy Paper III: To be published with the next LTS release of astropy core (v5.0, Fall 2021)
9 stars 38 forks source link

rephrase around mixin #126

Closed nstarman closed 2 years ago

nstarman commented 2 years ago

Addresses #124

This PR removes mentioning the word mixin, restructuring a paragraph to make this change.

In Python, a canonical mixin is any orthogonal class implementing a related set of methods that is used via multiple inheritance to provide functionality to many child classes without intending a relationship between those classes. More generally this sometimes becomes a little confused with classes that duck-type some behaviors and have some interoperability layer. This latter and looser definition is why the the protocol for table supporting many different column types is referred in the docs as mixin columns. It's isn't really, so I've rephrased to avoid mentioning confusing jargon.

Signed-off-by: nstarman nstarkman@protonmail.com

dhomeier commented 2 years ago

Thanks for the clarification! I agree that it makes sense to remove the term mixin from the section heading, but don't think we can entirely avoid it through the remainder of the text. After all it appears very prominently at the core of table.serialize and table.ndarray_mixin, so it's probably appropriate to explain its usage, even if not strictly true to the definition or the standard Python usage. Generally replacing mixin with mixing (or introducing a new term compin per your suggestion in https://github.com/astropy/astropy-v5.0-paper/discussions/124#discussioncomment-2622469, which will need its own discussion) may rather add to the confusion as long as mixin is so prevalently used in code and docs as well as in general discussion. Is the form introduced in collections.abc really the only "canonical" way to use it in Python? I had based my original question merely on https://en.wikipedia.org/wiki/Mixin, thus could not clearly make that connection either. But for one of the relevant cases here, registering a dask array as a "mixin column" as it is called there, does make it an object that behaves like a table.Column without actually inheriting from Column/BaseColumn (and thus np.ndarray); insofar this seems to meet the criteria of a "mixin" in its more general sense?

pllim commented 2 years ago

This missed the initial submission. @adrn said this will be part of a resubmission. Not sure if we want to wait to merge or what.

dhomeier commented 2 years ago

Yes, it was clear this would not make the first submission, needing some more discussion. Let's just see how far we can get before resubmission time comes around.

pllim commented 2 years ago

If this is pending discussion, should we turn it into draft for now?

adrn commented 2 years ago

I think these changes look good to me! @dhomeier and @nstarman: would you be happy to merge this?

dhomeier commented 2 years ago

The rephrased version looks very good to me – was mainly waiting if @mhvk had anything to add yet. And maybe the "mixin column" as defined in Astropy is not necessarily that completely disjunct from the canonical Python "mixin", if there even is such a thing – from references like https://www.linuxjournal.com/article/4540 or https://www.thedigitalcatonline.com/blog/2020/03/27/mixin-classes-in-python/#mixin-classes it looks to me like multiple inheritance is one possible realisation of the mixin concept, but does not have to be the only one. But I haven't thought of a better formulation, and the current wording would be entirely acceptable.