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

use extinction_total_to_selective_ratio func to get extinction from EBV #1048

Merged
merged 2 commits into from Dec 14, 2020

Conversation

julienguy
Copy link
Contributor

Use extinction_total_to_selective_ratio func to get extinction from EBV for the fit of standard stars.

@julienguy
Copy link
Contributor Author

(needs first to merge the PR with same name in desiutil)

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

It's nice that this has the added bonus of removing some trailing whitespace.

A few comments below, but none are blocking factors for a PR as the code works based on a test exposure: 67289 from Dec 12th, 2020 done in SPECPROD=daily at NERSC. outputs: /global/cfs/cdirs/desi/spectro/redux/daily/exposures/20201212/00067289/
log: /global/cfs/cdirs/desi/spectro/redux/daily/run/scripts/night/20201212/science-20201212-00067289-a0123456789-37239928.log

Comment 1:
You could add comments to describe what the r_band and a_band refer to/mean.

Comment 2:
Is there ever a chance that these inputs will be lists that don't understand boolean indexing? It currently works, so as long as they are always arrays, the logic will work.

Comment 3:
You could also add a short comment on the dimensions of each. I figured it out but it took a minute to parse together the dimensions of everything.

@julienguy
Copy link
Contributor Author

About your 'Comment 2', the fibermap type is a table, being an attribute of the class Frame. And star_unextincted_mags[band] is a numpy array declared in the function so they always accept boolean indexing.

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. The added comments are informative. I was more asking about the shapes of the input/output arrays than in physical units when I referred to "dimensions," but it's not an important issue. I'm okay with merging if Stephen gives a thumbs up

@sbailey
Copy link
Contributor

sbailey commented Dec 14, 2020

Thanks for cross checks and additional comments. Merging now, not waiting for Travis.

@sbailey sbailey merged commit 9d67543 into master Dec 14, 2020
@sbailey sbailey deleted the extinction_coeffs branch December 14, 2020 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants