add extinction_total_to_selective_ratio function #157
Conversation
py/desiutil/dust.py
Outdated
| def extinction_total_to_selective_ratio(band , photsys) : | ||
| """Return the linear coefficient R_B = A(B)/E(B-V) where A(B) = -2.5*log10(transmission in B band), | ||
| for band B in 'G','R' or 'Z', the optical bands of the legacy surveys. photsys = 'N' or 'S' specifies | ||
| the survey (BASS+MZLS or DECALS) |
There was a problem hiding this comment.
I would recommend specifying in here what "E(B-V)" means. I believe for DESI purposes it means E(B-V)_SFD, before any recalibrations to that quantity. One could imagine an rv argument to this function in some future world where we have a high latitude R(V) map.
There was a problem hiding this comment.
would an extra argument ebv_source with default = "SFD" would be fine? If not please suggest an alternative.
There was a problem hiding this comment.
I clarified the comment that E(B-V) means SFD, crossing commits with Julien's comment. OK to add ebv_source with default "SFD", but let's wait on rv option until we actually have code that could do something with it.
I also added unit tests for completeness.
There was a problem hiding this comment.
I am merging as is for now. We can go back to this in the future when we use different maps / calibrations for dust extinction.
There was a problem hiding this comment.
Sounds good. I'm happy with just specifying that E(B-V) means SFD in the docstring.
py/desiutil/dust.py
Outdated
| scalar, total extinction A(band) = -2.5*log10(transmission(band)) | ||
| """ | ||
|
|
||
| # fitted from the imaging data (using fibermaps of 2020/03/15) |
There was a problem hiding this comment.
This comment isn't very clear on where these data actually come from. Can additional details be added?
Also, are these numbers likely to change over the course of the survey? If so, do they have to be hard-coded in this file?
There was a problem hiding this comment.
Lots of meetings today so interleaving coding, commenting, and delays. Let's not move params into a separate file unless we find that is really needed. e.g. simply moving them into yaml file within desiutil would still require a new desiutil tag to update them so is effectively the same thing as hardcoding unless they need to be used in a different function to (I don't think they do).
There was a problem hiding this comment.
OK, I get that, but can some details about where the parameters actually come from would be very useful for long-term maintenance.
There was a problem hiding this comment.
Also, it would be great if a change log entry could be added to the PR.
|
@julienguy you're not going to address my review? |
|
@weaverba137 I was having a coffee :-) . I added some comment with link to DR8 documentation and edited the change log. |
|
@julienguy, it looks like you never synced your master checkout, because the change log comment is not under version 3.1.1. The comment on the numbers in the code looks fine though. |
|
Is this PR blazingly urgent? I'm worried people might be stepping on each other's toes here. |
|
Slipping in one more change: I added |
|
@weaverba137 you are right that we might be stepping on toes, but this PR is urgent because it is needed for desihub/desispec#1048 which is needed to process post-restart science data, i.e. we need it for tonight. :) |
|
Fair enough. Do you need this in a tag for tonight? |
|
good for me |
|
@weaverba137 I'll take care of any tags needed (not strictly needed for tonight, but needed for 20.12 release which is also imminent). |
add extinction_total_to_selective_ratio function