Standardize units (extended) #284

drvdputt · 2024-05-09T20:40:32Z

This includes the exact same commits as #259 , but adds a few extra changes

Units for 'power' columns. Set to 'amplitude units' for now, i.e. units.intensity or MJy / sr. After the "power gaussian" and "power drude" have been implemented, we will switch this to units.intensity_power
error message that makes clear that only units compatible with MJy / sr are supported
conversion of the spectrum and wavelengths to internal units when the user input is parsed

jdtsmith

Looks good overall. Let's save the docstring stuff for another PR. I'd like to merge this one first, then can try for #283 and fix conflicts.

jdtsmith · 2024-05-09T20:57:28Z

pahfit/features/features.py

+      The value and bounds as a 3 element tuple (value, min, max).
+          Any missing bound is replaced with the numpy `masked' value.


These doc changes seem out of bounds here. And this last one is not true. Any missing value is replaced.

Maybe I can rebase and drop the "docstring improvements" commit (35944ee)

If you use magit this is the perfect opportunity for branch spin-out.

Sounds interesting, but I love the interactive rebase interface of magit too ;). Rebased branch has been pushed.

jdtsmith · 2024-05-09T20:57:48Z

pahfit/features/features.py

+
+    Note that each parameter has an associated `kind', and that each
+    kind has an associated set of allowable parameters (see
+    `KIND_PARAMS`.


Missing close )

jdtsmith · 2024-05-09T20:58:53Z

pahfit/features/features.py

-        If reading a YAML file, read it in as a science pack and
-        return the new table. Otherwise, use astropy's normal Table
+        Parameters
+        ----------
+
+          file : str
+              The name of the file to read, either a full valid path,
+              or named file in the PAHFIT science_packs directory.
+
+        Returns
+        -------
+          table : Features
+              A filled `~pahfit.features.Features` table.
+
+        Notes
+        -----
+        If reading a YAML file, reads it in as a science pack and
+        return the new table. Otherwise, uses astropy's normal Table


Let's set aside all the docstring improvements to increase the chance that #283 and this one merge cleanly. Can do those in a subsequent PR.

jdtsmith · 2024-05-09T20:59:50Z

pahfit/features/features.py

+            if kind == "_ratios":
+                continue


I guess this is my first foray into tied parameters. I suppose we can leave it in as harmless for now.

jdtsmith · 2024-05-09T21:06:06Z

pahfit/model.py

+        The unit of the input spectrum has to be a multiple of MJy / sr,
+        the internal intensity unit. The output of this function


We want to allow flux units at some point. FYI, I haven't really looked much through Model, so don't get too wedded to any particular aspects.

jdtsmith · 2024-05-09T21:06:21Z

pahfit/model.py

        """
+        if not spec.flux.unit.is_equivalent(units.intensity):
+            raise PAHFITModelError("PAHFIT only supports intensity units, i.e. convertible to MJy / sr.")


drvdputt · 2024-05-09T21:32:14Z

I do realize now that a lot of our test spectra are loaded in Jy units... So all the test are failing there because of my new MJy / sr requirement. I will try to adjust the read_spectrum function so that it converts the example spectra.

jdtsmith · 2024-05-09T21:50:09Z

I guess the issue of dual support for flux or intensity units is that you ought to be able to apply a model to any spectrum? And that the spectrum "arrives late" on use.

One idea is to have the units for power column be written on output from a fit. I.e. have the table adapt to the thing being fitted. So it can start with no power or tau units, then as fits occur, if the units differ, have them update. Or be strict: the user must decide when making a Model whether they want flux or intensity units. The column gets updated, and any cross-species uses results in an error. It's "frozen". One strange thing about the model is that naturally tau_star and tau_MBB are unitless, and they scale spectra components with real intensity units. I don't even remember what I did about that originally. We didn't use those for much.

drvdputt · 2024-05-09T21:58:07Z

I have pushed a temporary fix to read_spectrum() (in helpers.py). It assumes a solid angle of 9 square arcseconds to turn the flux_density spectra into intensity. This at least makes the tests work, and should be removed once dual unit support is achieved, so that the example data are more "true to life" again.

Once the power-based features are implemented, I will have a better grip on how to design this. Somehow, the power-based feature classes will need to be configurable for both units. The unit type can definitely be an option at the initialization of Model, just like other keywords e.g. to choose the fitter backend. These keywords can then be passed to configure Fitter in the right way, so that it can determine how to set up the power-based components (in practice, this will come down to setting a few scaling factors for the evaluation function).

jdtsmith · 2024-05-10T03:03:30Z

Switched this to target dev; let me know when you feel it's ready.

drvdputt · 2024-05-10T15:17:40Z

The tests work on my machine with astropy==6.0.1, but I found that astropy 6.1.0 causes an issue with using features_table.loc[feature_name]. I will fix this issue in one of the other pull requests, before we merge dev into master.

So I think we're good to go here.

jdtsmith · 2024-05-10T18:58:04Z

OK merged. We do need to get all our tox tests working at some point.

drvdputt mentioned this pull request May 9, 2024

Simplify feature bounds using np.nan instead of masking #283

Merged

jdtsmith reviewed May 9, 2024

View reviewed changes

jdtsmith mentioned this pull request May 9, 2024

Standardize units #259

Closed

drvdputt force-pushed the units-standard branch from 9f03011 to 617a15a Compare May 9, 2024 21:27

jdtsmith and others added 12 commits May 9, 2024 18:13

Mask unused value for linter

646ffd1

Expose KIND_PARAMS and PARAM_UNITS as module level constants

6bd5e1d

UNITS: remove fwhm, add solid_angle

3d0aae5

Bug: Enum members need .value to extract the unit value

a274097

units: switch to simple module constants

548ded1

Remove 'tied' param/group type (for another PR)

8d637c4

_index_table: re-add

1157f89

linter happiness

b4a5ba4

Set power unit to intensity_power amplitude for now

21c61f0

Convert input spectrum to internal units, error if not ~ MJy sr-1

e7e5c70

State that MJy / sr unit restriction is temporary

1a3360b

Convert example spectra to MJy / sr (temporary fix)

4573a35

drvdputt force-pushed the units-standard branch from e828bae to 4573a35 Compare May 9, 2024 22:13

jdtsmith changed the base branch from master to dev May 10, 2024 03:02

jdtsmith merged commit 6ec9596 into PAHFIT:dev May 10, 2024
4 of 15 checks passed

drvdputt deleted the units-standard branch June 25, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize units (extended) #284

Standardize units (extended) #284

drvdputt commented May 9, 2024

jdtsmith left a comment

jdtsmith May 9, 2024

drvdputt May 9, 2024

jdtsmith May 9, 2024

drvdputt May 9, 2024

jdtsmith May 9, 2024

jdtsmith May 9, 2024

jdtsmith May 9, 2024

jdtsmith May 9, 2024

jdtsmith May 9, 2024

drvdputt commented May 9, 2024

jdtsmith commented May 9, 2024 •

edited

Loading

drvdputt commented May 9, 2024

jdtsmith commented May 10, 2024

drvdputt commented May 10, 2024

jdtsmith commented May 10, 2024

		The value and bounds as a 3 element tuple (value, min, max).
		Any missing bound is replaced with the numpy `masked' value.

		The unit of the input spectrum has to be a multiple of MJy / sr,
		the internal intensity unit. The output of this function

Standardize units (extended) #284

Standardize units (extended) #284

Conversation

drvdputt commented May 9, 2024

jdtsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvdputt commented May 9, 2024

jdtsmith commented May 9, 2024 • edited Loading

drvdputt commented May 9, 2024

jdtsmith commented May 10, 2024

drvdputt commented May 10, 2024

jdtsmith commented May 10, 2024

jdtsmith commented May 9, 2024 •

edited

Loading