Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor template/archetype model eval #293

Merged
merged 5 commits into from
Apr 25, 2024

Conversation

sbailey
Copy link
Collaborator

@sbailey sbailey commented Apr 19, 2024

@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

  • I renamed "COEFFTYPE" to "FITMETHOD" in anticipation of issue COEFF: track PCA/NMF vs. Archetypes vs. Legendre separately #274 splitting archetype coefficients into a separate column from template coefficients, and I also promoted that into being written out as part of the regular redrock REDSHIFTS table, not just in the model file.
  • for archetypes with nearest neighbors, I joined the subtypes with a semicolon instead of an underscore so that e.g. SUBTYPE='ELG_23;ELG_40' instead of SUBTYPE='ELG_23_ELG_40'. That makes it easier to split the SUBTYPE string into the individual archetype subtypes.

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

  • wavedict, wave_dict, and dwave all being dictionaries of wavelength grids differing only by their keys
  • Archtype.get_best_archetype_model having to know about templates just in case it is asked to evaluate something that isn't an archtype.
  • New functions get_best_model_spectra, eval_model_for_one_spectra , and eval_tdata having significant conceptual overlap with previously existing eval functions, but not actually using those. Instead of pulled pieces of those into the the other eval functions.

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

  • wavehash vs. camera bands: For DESI, all spectra of a given band have the same wavelengths so DistTargetsDESI can just use the band as the wavehash.
  • wavehash out of order compared to camera bands (this was due to a set operation changing the order).

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.

@sbailey sbailey requested a review from abhi0395 April 19, 2024 18:34
@coveralls
Copy link

coveralls commented Apr 19, 2024

Coverage Status

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

Copy link
Member

@abhi0395 abhi0395 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks for cleaning out the messiness of my best_fit_model branch. I have also opened the PR#299, which fixes the inconsistent definition of legendre wavelength basis.

I think, merging this branch with that should fix legendre wavelength basis definition here as well.

@sbailey sbailey merged commit a97b632 into best_fit_model Apr 25, 2024
12 checks passed
@sbailey sbailey deleted the best_fit_model_refactor branch April 25, 2024 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants