-
Notifications
You must be signed in to change notification settings - Fork 9
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
Dust #169
Conversation
…n(), add dust.ebv_sfd_scaling = 0.86 ; add function dust_transmission(wave,ebv_sfd)
Trying to track down what values I think should be right... this is what tractor uses: I have not been able to track down the provenance of the numbers you are currently using for DECaLS. So, focusing on the numbers I do understand (3.214, 2.165, 1.211), and comparing those with the numbers you compute for DECam (3.678, 2.459, 1.376), I get that these are different by a fixed scale factor of 0.88 (c.f. SF11 rescaling of SFD by 0.86). That's looks close, but it's better than that---the difference between 0.88 and 0.86 is because 0.86 is calculated for a galaxy source spectrum more appropriate to the original SFD calibration, rather than for a 7000 K F star. So I think what you're doing is basically right. I recommend applying the SF11 calibration in the following way: As part of preparing this comment, I tried to trace the history of the number 1.029 again. Here's more information than you want. You should probably just stop reading here. You have been warned. 1.029 is 0.78*1.319, where 0.78 is from the SF11 appendix and 1.319 is a magic number buried in dust_intfilter.pro, a bit of IDL used to compute the coefficients for SFD98. 1.319 is supposed to represent A(1 micron)/E(B-V) for the O'Donnell extinction curve. I can't reproduce that number myself. But it's not far from ext_odonnell(10000)/(ext_odonnell(4404)-ext_odonnell(5428)) = 1.326, where 4404 and 5428 are the effective wavelengths cited for the Landolt B and V filters in the SFD appendix. And if I just treat it as a magic constant I can reproduce the SFD appendix numbers. Presumably SFD was using some other definition of E(B-V), possibly non-monochromatic, or just using different wavelengths. In other words, in the parameterization of SF11, the recalibration is a factor of 0.78, but much of that is because O'Donnell and F99 extrapolate from B-V to 1 micron differently. |
@julienguy, any chance of adding unit tests to cover these changes? In general, what is the relationship between this PR and #166? |
…rveys ; modify normalization of fitzpatrick reddening curve in dust_transmission
(note the differences have to be multiplied by E(B-V) which is typically <0.05 ) |
I have also removed the scale factor |
…make the choice to keep the same values as in the legacy surveys
FWIW, in the event we wanted to diagnose the DECam g band difference more, my guess is that this is due to my using an airmass of 1.4 here: |
@julienguy are you actively working on the documentation problem, or are you asking someone else to do that? |
I would appreciate if you have a look at it. |
OK, documentation fixed. |
@julienguy, it looks like you got tests to pass by commenting out the tests. I don't think that's a very good practice. Are you planning to restore the tests? |
Have you actually seen why I commented out the tests? |
@julienguy, no, in fact there are no additional comments in test_dust.py that explain why the tests were commented out. I also don't see any discussion of that in this thread. |
…lective ratios R_x , and one needs to set option match_legacy_surveys=True to get consistent results with legacy surveys
After discussion with Stephen, the default is now the most accurate estimate of the total to selective ratios.
|
New doc failures. I let you @weaverba137 fix this if you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not evaluated the correctness of this PR, but some structural comments:
- let's directly support Gaia here instead of treating it as a special case on the desispec side
- While we're at it, do we already have coefficients for WISE and other DECam (or even SDSS and PanSTARRS?) bands that we could plug in here to make this more generically useful even if we're not using them yet? I don't want to add feature creep, but it could head of future proliferation of dust-like code (like you are cleaning up in Dust desispec#1246).
- since there are some subtleties here about the default behavior, let's be more proactive with the tests and check:
- results match Legacy Surveys if
photsys='S'
regardless of whethermatch_legacy_surveys=True
orFalse
. Test with a few hardcoded (but documented provenance) examples taken from DR9 catalogs spanning small, medium, large E(B-V), - for
photsys='N'
, results match Legacy Surveys ifmatch_legacy_surveys=True
but does not match LS ifmatch_legacy_surveys=False
- calling with
PHOTSYS='G'
(Gaia) works (or at least doesn't crash)
- results match Legacy Surveys if
"Z_N":1.2110, | ||
"G_S":3.2829, | ||
"R_S":2.1999, | ||
"Z_S":1.2150} | ||
assert(band.upper() in ["G","R","Z"]) | ||
assert(photsys.upper() in ["N","S"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also support PHOTSYS='G' (Gaia) since that appears in the targeting catalogs as well, with allowable band values of "G", "BP", and "RP". This would move desispec.scripts.stdstars.unextinct_gaia_mags
logic over here instead of having Gaia treated as a special case on the desispec side (which I think it still is there in desihub/desispec#1246).
For consideration: older targets/fiberassign files also have PHOTSYS='' meaning Gaia; we should track down whether that is recent enough for us to care (Dec 2020 or later), but I haven't done that yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segasai could you help with moving the Gaia extinction code out of desispec and into this PR branch so that desiutil.dust
is our one-stop-shop for extinction corrections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what I can do (on a timescale of few days)
@@ -482,3 +670,78 @@ def ebv(*args, **kwargs): | |||
south=kwargs.get('south', "SFD_dust_4096_sgp.fits"), | |||
scaling=kwargs.get('scaling', 1.)) | |||
return m.ebv(*args, **kwargs) | |||
|
|||
def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this main
leftover from debugging, or is it still useful? I'm ok with sometimes hiding super-specialized scripts under if __name__ == "__main__":
blocks under otherwise module code, but if you do want to retain this please add some brief comments about what it does and add a basic argparse
so that python dust.py --help
would be informative without actually running anything.
I have committed an implementation of the Fitzpatrick reddening law.
I added
dust.ebv_sfd_scaling = 0.86
and a functiondust_transmission(wave,ebv_sfd)
.I compare
extinction_total_to_selective_ratio
and synthetic mags of an extincted dummy spectrum using filters from speclite in a main function (that I can remove if some of you don't like it).I do not obtain a very good agreement so it would be great if an expert has a look at it.