search

desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
35 stars 24 forks source link

MgII afterburner broken due to recent redrock change #2167

Closed moustakas closed 7 months ago

moustakas commented 7 months ago

@sbailey I'm surprised we haven't seen this crash in the daily pipeline yet, but I'm fairly certain that https://github.com/desihub/redrock/pull/271 broke the MgII after-burner by removing the template_dir argument of redrock.find_templates (see here), which is used by desispec.mgii_afterburner.load_redrock_templates here.

I can easily fix this in a PR but there are a few different ways of doing so. First, do you want me to fix this in redrock or desispec? And if in redrock, do we want to add backwards compatibility for the template_dir argument?

akremin commented 7 months ago

@moustakas the pipeline hadn't tripped over it because we have had bad weather recently and haven't had any science data since the change.

I think we should proceed with modifying the mgii code in desispec given that Stephen's change was probably an intentional refactor.

I will submit a PR and self merge.

akremin commented 7 months ago

This is fixed via desispec in PR #2168

sbailey commented 7 months ago

Ugh, apologies for breaking this with a cosmetic fix as part of the Redrock templates refactor. Thanks to the attention/debugging from @moustakas and @abhi0395 and for the weekend fix from @akremin . My penance shall be to write a unit test that would have caught the incompatibility in the first place.