BlackBody model: deprecate non-dimensionless scale and add support for flux units by kecnry · Pull Request #12304 · astropy/astropy

kecnry · 2021-10-27T15:50:54Z

Description

This pull request deprecates non-dimensionless scale in models.BlackBody and replaces the functionality with output_units, with added support for flux (density) units. This also fixes the case where dimensionless (but not unscaled) units are passed to scale.

Until support for non-dimensionless scale is removed in the future, a warning is raised at both __init__ and future calls to bolometric_flux (both warning about deprecation and the possible ambiguous treatment). The input scale quantity is first converted to one of the supported native output units and stored internally as a float, with the originally passed units being passed to the new output_units parameter. In doing so, this also "fixes" the case mentioned in #11547 (comment)... although the expected behavior is still ambiguous (thus the deprecation). If the units on scale are flux units, then pi is also removed as it is added internally within evaluate (see example below).

Revisiting the cases in #11547:

from astropy.modeling.models import BlackBody
from astropy import units as u
import numpy as np

T = 3000 * u.K
r = 1e14 * u.cm
DL = 100 * u.Mpc
scale = np.pi * (r / DL)**2

print(BlackBody(temperature=T, scale=scale).bolometric_flux)
print(BlackBody(temperature=T, scale=scale.to_value(u.dimensionless_unscaled)).bolometric_flux)

now returns the expected output (the passed scale with units of cm ** 2 / Mpc **2 are converted to dimensionless_unscaled before being applied):

4.823870774433646e-16 erg / (cm2 s)
4.823870774433646e-16 erg / (cm2 s)

The second example now gives the expected results (change from previous behavior) with deprecation warnings for passing non-dimensionless quantities to scale:

from astropy.modeling.models import BlackBody
from astropy import units as u
from matplotlib import pyplot as plt
import numpy as np

nu = np.geomspace(0.1, 10) * u.micron
bb1 = BlackBody(temperature=3000*u.K, scale=1*u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb2 = BlackBody(temperature=3000*u.K, scale=1*u.J/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb3 = BlackBody(temperature=3000*u.K, scale=1e7*u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))

fig, ax = plt.subplots()
ax.set_xscale('log')
ax.set_yscale('log')
ax.plot(nu.value, bb1(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), lw=4, label='bb1')
ax.plot(nu.value, bb2(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), lw=4, label='bb2')
ax.plot(nu.value, bb3(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), label='bb3')
ax.legend()
fig.savefig('test.png')

The new preferred (non-ambiguous) version of this same example would be:

from astropy.modeling.models import BlackBody
from astropy import units as u
from matplotlib import pyplot as plt
import numpy as np

nu = np.geomspace(0.1, 10) * u.micron
bb1 = BlackBody(temperature=3000*u.K, scale=1.0, output_units=u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb2 = BlackBody(temperature=3000*u.K, scale=1*u.J/u.erg, output_units=u.J/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb3 = BlackBody(temperature=3000*u.K, scale=1e7, output_units=u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))

fig, ax = plt.subplots()
ax.set_xscale('log')
ax.set_yscale('log')
ax.plot(nu.value, bb1(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), lw=4, label='bb1')
ax.plot(nu.value, bb2(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), lw=4, label='bb2')
ax.plot(nu.value, bb3(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), label='bb3')
ax.legend()
fig.savefig('test2.png')

and the previous (unexpected) behavior can now be reproduced with (note the difference in scale passed to bb2: one with unit conversion, the other without):

from astropy.modeling.models import BlackBody
from astropy import units as u
from matplotlib import pyplot as plt
import numpy as np

nu = np.geomspace(0.1, 10) * u.micron
bb1 = BlackBody(temperature=3000*u.K, scale=1.0, output_units=u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb2 = BlackBody(temperature=3000*u.K, scale=1.0, output_units=u.J/(u.cm ** 2 * u.s * u.Hz * u.sr))
bb3 = BlackBody(temperature=3000*u.K, scale=1e7, output_units=u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr))

fig, ax = plt.subplots()
ax.set_xscale('log')
ax.set_yscale('log')
ax.plot(nu.value, bb1(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), lw=4, label='bb1')
ax.plot(nu.value, bb2(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), label='bb2')
ax.plot(nu.value, bb3(nu).to_value(u.erg/(u.cm ** 2 * u.s * u.Hz * u.sr)), label='bb3')
ax.legend()
fig.savefig('test3.png')

Additionally, flux units are now supported by passing valid flux units to output_units, with a factor of pi multiplied in addition to the dimensionless scale:

from astropy.modeling import models
import astropy.units as u

import numpy as np

nu = np.geomspace(0.1, 10) * u.micron

bb = models.BlackBody(temperature=3000*u.K, scale=1.0, output_units=u.erg / (u.cm ** 2 * u.s * u.Hz))
bb(nu)

To be consistent, passing flux units to scale are temporarily supported (with a deprecation warning), in which case the units are converted as above but a factor of pi is removed (as solid angle are in the units):

from astropy.modeling import models
import astropy.units as u

import numpy as np

nu = np.geomspace(0.1, 10) * u.micron

bb = models.BlackBody(temperature=3000*u.K, scale=np.pi*u.erg / (u.cm ** 2 * u.s * u.Hz))
bb(nu)
print(bb.scale) # set internally as 1.0

Fixes #11547

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

* move towards scale being a dimensionless parameter with deprecated support for stripping any pass unit and passing to output_units instead. * output_units can be SNU (default), SLAM, FNU, FLAM (or equivalent) * warnings when passing scale with units AND when calling bolometric flux after passing scale with units.

* but not exactly one of the supported "native" output units. As noted by astropy#11547 (comment) this can result in confusing behavior. The behavior is much clearer when forcing to separate the unitless scale from the desired output_units.

and cleanup comments

* see case described in astropy#11547 (comment) * if scale is passed with units that are equivalent to the valid output units (SNU, SLAM, FNU, FLAM), the scale is first converted to the native units and adopted as a float, with the original units adopted as output_units. * new behavior is different, but consistent with stripping pi out of passed flux units

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

astropy/modeling/physical_models.py

astropy/modeling/tests/test_physical_models.py

docs/changes/modeling/12304.api.rst

lpsinger

The new feature in which there is a conditional multiplication by pi depending on the value of output_units is an added footgun. I suggest removing it. The patch will become drastically simpler without it.

As I explained in #11547 (comment), the black body is not a special case because you need the factor of pi whenever you have an isotropic spherical emitter.

A case could be made that there should be an opt-in equivalency in astropy.units for converting from SNU to FNU assuming isotropy, but that conversion does not belong in the BlackBody class.

kecnry · 2021-10-27T19:58:45Z

Although I'm not entirely opposed to removing the multiplication of pi (I think its more of a philosophical decision than anything else and can change it if that's the consensus), I think the factor of pi will have to show up somewhere - it's either considered not to be included in scale and then multiplied internally in evaluate, or considered absorbed into scale but then divided in bolometric_flux. Even if we split into two classes, we still have this design "issue" unless we remove support for bolometric_flux or decide not to add support for FNU/FLAM at all (in which case the user has to worry about scaling the two outputs differently).

If the decision is that FNU/FLAM support does not belong in BlackBody for these reasons, I can isolate just the fix for dimensionless (but not unscaled) to at least fix that bug. I'm not sure splitting scale and output_units makes sense if only supporting SNU/SLAM, but if it's not split I'm unsure where that would leave the second example above. That really requires the more verbose syntax of output_units to make it clear what will happen under-the-hood.

kecnry · 2021-10-27T20:23:26Z

#9450 and #6505 are relevant to the idea to handle all unit conversions with equivalencies

lpsinger · 2021-10-27T21:27:11Z

The factor of pi is present in synphot.models.BlackBody1D. I think that's where it belongs.

astrofrog · 2021-11-01T13:20:24Z

I am re-milestoning to v5.1 as we've now branched v5.0.x

karllark · 2021-11-10T14:13:43Z

The factor of pi is one that should definitely be discussed separately. There are pros and cons on this topic and there is no easy answer. We should do something quite explicit to make it clear when the factor of pi is being included or not.

github-actions · 2022-03-27T05:37:27Z

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.

pllim · 2022-04-21T21:27:11Z

This PR is marked stale and there are conflicts, so I am remilestoning. FYI.

github-actions · 2022-04-27T05:41: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.

kecnry added 7 commits October 26, 2021 18:04

handle dimensionless (but not unscaled) input scale

2ae61c8

update docstrings

d211657

and cleanup comments

update tests

c2b33df

remove trailing whitespace

8a4328b

github-actions bot added the modeling label Oct 27, 2021

pllim added this to the v5.0 milestone Oct 27, 2021

pllim added the Refactoring label Oct 27, 2021

github-actions bot reviewed Oct 27, 2021

View reviewed changes

pllim requested review from eteq, karllark and lpsinger October 27, 2021 16:08

pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Oct 27, 2021

pllim reviewed Oct 27, 2021

View reviewed changes

astropy/modeling/physical_models.py Outdated Show resolved Hide resolved

pllim reviewed Oct 27, 2021

View reviewed changes

astropy/modeling/tests/test_physical_models.py Show resolved Hide resolved

add changelog entry

b2515ae

pllim reviewed Oct 27, 2021

View reviewed changes

astropy/modeling/tests/test_physical_models.py Outdated Show resolved Hide resolved

pllim reviewed Oct 27, 2021

View reviewed changes

docs/changes/modeling/12304.api.rst Outdated Show resolved Hide resolved

address review suggestions from pllim

1823c0d

lpsinger suggested changes Oct 27, 2021

View reviewed changes

kecnry mentioned this pull request Oct 28, 2021

BlackBody model: fix handling of scale units #12318

Merged

9 tasks

astrofrog modified the milestones: v5.0, v5.1 Nov 1, 2021

pllim mentioned this pull request Nov 23, 2021

BlackBody and units support within modeling plugin spacetelescope/jdaviz#953

Merged

9 tasks

github-actions bot added the Close? Tell stale bot that this issue/PR is stale label Mar 27, 2022

pllim modified the milestones: v5.1, v5.2 Apr 21, 2022

github-actions bot added the closed-by-bot Closed by stale bot label Apr 27, 2022

github-actions bot closed this Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

BlackBody model: deprecate non-dimensionless scale and add support for flux units#12304

BlackBody model: deprecate non-dimensionless scale and add support for flux units#12304
kecnry wants to merge 9 commits intoastropy:mainfrom
kecnry:modeling-bb-scale

kecnry commented Oct 27, 2021 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpsinger left a comment

Uh oh!

kecnry commented Oct 27, 2021

Uh oh!

kecnry commented Oct 27, 2021

Uh oh!

lpsinger commented Oct 27, 2021

Uh oh!

astrofrog commented Nov 1, 2021

Uh oh!

karllark commented Nov 10, 2021

Uh oh!

github-actions bot commented Mar 27, 2022

Uh oh!

pllim commented Apr 21, 2022

Uh oh!

github-actions bot commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

kecnry commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist for package maintainer(s)

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpsinger left a comment

Choose a reason for hiding this comment

Uh oh!

kecnry commented Oct 27, 2021

Uh oh!

kecnry commented Oct 27, 2021

Uh oh!

lpsinger commented Oct 27, 2021

Uh oh!

astrofrog commented Nov 1, 2021

Uh oh!

karllark commented Nov 10, 2021

Uh oh!

github-actions bot commented Mar 27, 2022

Uh oh!

pllim commented Apr 21, 2022

Uh oh!

github-actions bot commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kecnry commented Oct 27, 2021 •

edited

Loading