Closed moustakas closed 2 years ago
@dkirkby @moustakas as a heads up, I've added this to the DR1/Iron project to keep it on our radar for including in the code release for the Iron production.
This looks good, thanks! Can you also update docs/filters.rst
to describe the changes?
Regarding the naming, any reason not to use sdss2010
and sdss2010noatm
to be parallel with the existing decamDR1(noatm)
names?
Regarding the naming, any reason not to use sdss2010 and sdss2010noatm to be parallel with the existing decamDR1(noatm) names?
Thanks for the suggestion. My concern was the backwards-incompatibility: That if someone has been using the sdss2010
filters and applying the atmosphere themselves and we now make those the default, that it would cause issues. But if you're happy with making these the default (which is my preference) then I can make this change and also update the documentation.
I think this is the best solution overall since the docs always claimed sdss2010
included an atmosphere (so that hypothetical user presumably already knew they were wrong). Perhaps include a note in the doc page that this was wrong earlier?
@dkirkby I think this is ready for final review and merging.
This PR addresses #39 and #66 by adding SDSS filters which include the atmosphere at an airmass of 1.3. The group name nor the names of the individual filters are great, but let me know if you'd like something different @dkirkby.
This PR is a blocking factor for some of my other DESI work, so it'd be great if we could merge this within a day or two.
sdss2010atm filters
sdss2010 filters
For reference, this is the bit of code I wrote to parse the Doi et al. Table 4 (after mildly editing the file to only include the column names as a simple header):