Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redefine VEGAMAG and OBMAG so astropy can tell them apart #327

Closed
pllim opened this issue Apr 26, 2022 · 0 comments · Fixed by #331
Closed

Redefine VEGAMAG and OBMAG so astropy can tell them apart #327

pllim opened this issue Apr 26, 2022 · 0 comments · Fixed by #331

Comments

@pllim
Copy link
Contributor

pllim commented Apr 26, 2022

OBMAG = u.def_unit(
'obmag', u.mag, format={'generic': 'OBMAG', 'console': 'OBMAG'})
VEGAMAG = u.def_unit(
'vegamag', u.mag, format={'generic': 'VEGAMAG', 'console': 'VEGAMAG'})

As stated in astropy/astropy#13158 (comment) , the new astropy.modeling.models.Schechter1D takes magnitude as parameter but currently it cannot tell VEGAMAG and OBMAG apart. @mhvk advised the following:


[The] problem is that they now are both just aliases to u.mag, so the units system cannot know what to do. I think best might be to treat it a bit like ST and AB, and define units VEGA and OB - which both being units without a known number, and then define VEGAMAG = u.mag(VEGA) and OBMAB = u.mag(OB). Then one can no longer just mix and match those magnitude systems:

>>> from astropy import units as u
>>> VEGA = u.def_unit('VEGA')
>>> OB = u.def_unit('OB')
>>> VEGAMAG = u.mag(VEGA)
>>> OBMAG = u.mag(OB)
>>> (5 * VEGAMAG - 2.5 * VEGAMAG).to(u.one)
<Quantity 0.1>
>>> (5 * VEGAMAG - 2.5 * OBMAG).to(u.one)
UnitConversionError: ("'VEGA / OB' and '' (dimensionless) are not convertible",
'Did you perhaps subtract magnitudes so the unit got lost?')

Note that of course the user can still shoot themselves in the foot by forgetting what band a magnitude is in...


To go with this patch, relevant documentation and tests might also need updating. And this might be a breaking change, so should not be in a bugfix release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant