search

astropy / astropy

Astronomy and astrophysics core library
https://www.astropy.org
BSD 3-Clause "New" or "Revised" License
4.45k stars 1.78k forks source link

Should the model composition operator, `|`, support units changing #17302

Open WilliamJamieson opened 2 weeks ago

WilliamJamieson commented 2 weeks ago

This was proposed by #17127 and there are several comments expressing support for this change:

The issue is that changing this behavior will break the current API support. Namely, fitting the composition CompoundModel supporting the proposed change will break the current API by breaking the ability to infer the units from the parameters and the individual models. In particular, this will break the unit test: https://github.com/astropy/astropy/blob/2dee6835702668bbe05735e87b901769f751d2b5/astropy/modeling/tests/test_quantities_fitting.py#L204.

The issue here is that there is no clear way to make this change with a prior deprecation or warning of the impending change to end users. Due to this issue, we will need to be very careful about making this change.

This issue is intended to provide a place for discussion on how to proceed.

WilliamJamieson commented 2 weeks ago

As there are requests to have support this right now, I suggest we add an intermediate solution where the fix from #17127 is protected by a compound model property that has to be manually toggled by the user on the resulting model to turn on this behavior. A special modeling function can also be added to do this in a single step.

pllim commented 2 weeks ago

cc @nden @perrygreenfield @astrofrog @Cadair