desihub / gpu_specter

Scratch work for porting spectroperfectionism extractions to GPUs
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

Compare ex2d patch #30

Closed sbailey closed 4 years ago

sbailey commented 4 years ago

Opening a PR on behalf of @dmargala for final review. This PR adds test comparisons to specter.extract.ex2d.ex2d_patch, and adds options for whether the R-matrix calculation should be only internal to individual fibers, or include decorrelation across fibers. Thanks for the updates that make these within np.allclose level of agreement.

Briefly thinking about it more, my description that the decorrelation is for noise vs. signal isn't quite right either. It's more about whether the calculation should decorrelate across neighboring fibers or not. The default in this branch is good (only within a single fiber), and I like the structure of how the option is used, but I'd like to think a bit more about what the option names should be. I'm booked for the rest of today, but will try to comment more tonight.

To make progress in the meantime, I'm ok with either:

dmargala commented 4 years ago

Great! Thanks @sbailey

I just added an additional test so that both decorrelation modes are covered now.

I favor merging now the with expectation that the option name may change.