PAHFIT / pahfit

Model Decomposition for Near- to Mid-Infrared Spectroscopy of Astronomical Sources
https://pahfit.readthedocs.io/
18 stars 26 forks source link

Standardize units (extended) #284

Closed drvdputt closed 4 months ago

drvdputt commented 4 months ago

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

drvdputt commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

drvdputt commented 4 months ago

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 commented 4 months ago

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