desihub / redrock

Redshift fitting for spectroperfectionism
BSD 3-Clause "New" or "Revised" License
21 stars 13 forks source link

refactor template/archetype model eval #293

Closed sbailey closed 2 months ago

sbailey commented 2 months ago

@abhi0395 this is an update to your best_fit_model branch (PR #283) to address some of my maintainability concerns. I've made enough changes that I'm doing this as a PR to your branch rather than updating your branch directly. Although this version is quite a bit different from your original branch, I learned a lot from your work about the various corner cases with MPI vs. multiprocessing, wavehashes out of order compared to camera bands, etc. so thanks for that initial solution which guided me for what to watch out for.

In the end the output files are the same except

Big Caveat: the evaluated Archetype+legendre models are not correct, which I think is due to issue #291 with the inconsistent definitions of legendre basis wavelengths and whether they span the entire brz wavelength range or only the individual cameras. Let's get that sorted out separately, and then revisit this model evaluation to make sure it uses the same pieces. We don't need this for Jura, but we will need it for Archetype runs post-Jura.

More details

The goal here is to have Template.eval() and Archetypes.eval() "own" the concept of how to evaluate a model given coefficients and have rrdesi -> DistTargets.eval_models -> Target.eval_model use those. This simplifies the bookkeeping to avoid things like

I also updated some of the underlying messiness that made your original branch hard to implement:

Please take a look at this, and then let's discuss any items that you disagree with and/or have a better idea for how to implement. Thanks.

coveralls commented 2 months ago

Coverage Status

coverage: 37.789% (+0.8%) from 36.952% when pulling c006d4fc0635999b5cf3859a9b96517fa78dfc54 on best_fit_model_refactor into 6326df1d2300a27bb0df7bf1b65b9c13b7182236 on best_fit_model.