PAHFIT / pahfit

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

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

Closed jdtsmith closed 2 months ago

jdtsmith commented 2 months ago

Partially masked arrays are not really well supported, and required elaborate workarounds for pretty printing. Here we switch to using a structured array type (val, min, max fields), and represent "fixed" bounds with a pair of np.nan values for min and max, instead of masking them. I.e. nan is interpreted as the same as negative or positive infinity, unless both bounds are nan, which indicates a fixed parameter.

"Fully open" (unconstrained) bounds are expressed just as before, with (-np.inf, np.inf). But in general: don't do those ;).

jdtsmith commented 2 months ago

While working on the representation, I also stamped out the dtype printing on __repr__ (which is oddly on by default), and allow now a meta['pahfit_format'] on the table and/or individual columns, to affect the printed representation. E.g.

f.meta['pahfit_format']='.2f'
f['wavelength'].meta['pahfit_format']='.3f'

yielding:

...
    H2_S(7)     H2_lines           line              --             --  5.511 (fixed) 0.00 (0.00, ∞)              --              --       --
    H2_S(6)     H2_lines           line              --             --  6.109 (fixed) 0.00 (0.00, ∞)              --              --       --
    H2_S(5)     H2_lines           line              --             --  6.909 (fixed) 0.00 (0.00, ∞)              --              --       --
    H2_S(4)     H2_lines           line              --             --  8.026 (fixed) 0.00 (0.00, ∞)              --              --       --
...

To be documented somewhere.

drvdputt commented 2 months ago

It looks like all of this is working as intended. At least until the actual until the Features -> astropy model part.

We cannot really make all the tests succeed for this, without revising all the pieces of code that directly address the Features table (I tried to make things work for a bit, but there are more of these than I expected). Especially, I cannot justify revising all of base.py just to make the tests work, since I aim to delete it soon, when replaced by APFitter.

If we merge this, then we will have to continue in this broken state until I have integrated and adapted APFitter.

I suggest we merge the units pull request first (the extended version I just suggested, #284 ). In that case, the tests are still working, at least on my machine. And the plot rework #281 too.

After that, we can do the big break: merge this, and then include my internal API rework (which will be easier to adapt to the new Features format).

jdtsmith commented 2 months ago

This sounds good. What type of updates to APFitter do you mean? Features is still accessible in the same way, and in fact p[0] and p['val'] both work.

drvdputt commented 2 months ago

Things that do not work anymore

The new columns are a structured array, instead of a regular 2D array, so there are some restrictions.

jdtsmith commented 2 months ago

OK. Overall I think the structured array is much cleaner. I intend to merge your extend units PR #284 once finalized, then #281, then this #283.

jdtsmith commented 2 months ago

Also do not worry about soon-to-be-forgotten base.py.