search

PAHFIT / pahfit

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

Workarounds for issues with Features column definition and formatting #282

Closed drvdputt closed 3 weeks ago

drvdputt commented 2 months ago

There were a series of strange issues with the Features class in certain edge cases

To solve #274, both the Column and MaskedColumn definitions of Features were set to BoundedMaskedColumn. The formatting hacks for Features were revised to deal with the above change and with #273 and #266.

The new formatting method, takes the default output of the formatter (using if/else to cover all possible strings created for a 3-tuple), and modifies it through string replacements.

This pull request is independent from #281, so one can go in after the other (at most a simple rebase required).

jdtsmith commented 2 months ago

A very relevant conversation about the pretty-printing is here. You can see me dealing with the fact that the default printer omits the middle of a 3-tuple (like our bounded param).

Update: I'm pretty queasy about adding string hacks on top of hacks like this. Using structured arrays for the data without anything else will work, but right now any masked value makes Table mask the entire row's value (i.e. all "fixed" values show up as --). I think that's why I ended up with the current hack (temporarily lying about the shape of the underlying data). Let's see if we can resolve the issue you saw more simply. Update: simple solution in #283.

jdtsmith commented 2 months ago

I think this is entirely superseded by the much simpler #283; please have a look at that. Here's my proposal: let's merge

  1. 259

  2. 281 (once review is addressed)

  3. 283 (pending your review)

to dev, test that combo. If all's well on dev proceed with the next steps (targeting dev until we're happy).

drvdputt commented 2 months ago

Ok, I will review the above three pull requests. I'll post another comment here once I'm done with this, and then we'll pull those into dev. I will then test test some of my edge cases in dev, to see if we still need any extra hacks on top of #283.

jdtsmith commented 2 months ago

I think this has been superseded by #283 and can be closed, yes?

drvdputt commented 3 weeks ago

Yes, we have a different approach in the other pull request now. Closing.