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

Add rebinning to a constant resolution for spectra and extinction data #100

Merged
merged 18 commits into from
Mar 2, 2022

Conversation

karllark
Copy link
Owner

@karllark karllark commented Jan 12, 2022

Adds option to rebin to a specified constant spectral resolution for spectra and spectral extinction curves. Documentation for the basic classes and capabilities including the rebinning included.

As part of the testing of the rebinning, some changes to the plotting scripts were made. One change was to remove the forcing of all filenames to be lower case allowing for a wider range of filenames to be used. Another was to switch from using the filenames to using the saved names of the reddened and comparison stars in the extinction FITS files.

Finally, the wavenum capability added previously in PR #81 did not fully work with the plotting scripts. Updates were made to more fully support plotting versus 1/lambda. The missing plotting capabilities were revealed during the rebin testing.

Fixes a few minor bugs found during this work.

@karllark karllark added bug Something isn't working enhancement New feature or request documentation labels Jan 12, 2022
@karllark karllark marked this pull request as draft January 12, 2022 22:06
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Attention: Patch coverage is 70.94017% with 34 lines in your changes missing coverage. Please review.

Project coverage is 47.56%. Comparing base (d416b35) to head (a8f8a2e).
Report is 85 commits behind head on master.

Files Patch % Lines
measure_extinction/plotting/plot_ext.py 63.82% 17 Missing ⚠️
measure_extinction/plotting/plot_spec.py 37.50% 10 Missing ⚠️
measure_extinction/extdata.py 78.78% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   46.52%   47.56%   +1.03%     
==========================================
  Files          19       19              
  Lines        2577     2670      +93     
==========================================
+ Hits         1199     1270      +71     
- Misses       1378     1400      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@karllark karllark marked this pull request as ready for review January 14, 2022 22:34
Copy link
Collaborator

@mdecleir mdecleir left a comment

Choose a reason for hiding this comment

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

These changes look great!
I mostly indicated typos and have a few minor comments.

docs/code_capabilities.rst Outdated Show resolved Hide resolved
docs/code_capabilities.rst Outdated Show resolved Hide resolved
docs/code_capabilities.rst Outdated Show resolved Hide resolved
docs/code_capabilities.rst Outdated Show resolved Hide resolved
docs/code_capabilities.rst Show resolved Hide resolved
measure_extinction/plotting/plot_spec.py Show resolved Hide resolved
measure_extinction/plotting/plot_spec.py Outdated Show resolved Hide resolved
measure_extinction/tests/test_rebin.py Outdated Show resolved Hide resolved
measure_extinction/tests/test_rebin.py Show resolved Hide resolved
measure_extinction/tests/test_rebin.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mdecleir mdecleir left a comment

Choose a reason for hiding this comment

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

Thanks for fixing/replying to my comments! Here are a few more minor things.
(For some reason, some comments are part of this review, and others are individual comments, not sure how that happened, but I hope you will find everything.)

measure_extinction/extdata.py Show resolved Hide resolved
measure_extinction/extdata.py Outdated Show resolved Hide resolved
measure_extinction/extdata.py Outdated Show resolved Hide resolved
measure_extinction/stardata.py Outdated Show resolved Hide resolved
measure_extinction/stardata.py Outdated Show resolved Hide resolved
@karllark
Copy link
Owner Author

Ok to merge now?

Copy link
Collaborator

@mdecleir mdecleir left a comment

Choose a reason for hiding this comment

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

Just two more typos...

measure_extinction/extdata.py Outdated Show resolved Hide resolved
@@ -816,7 +816,7 @@ def rebin_constres(self, waverange, resolution):

Parameters
----------
waverange : [float, float]
waverange : 2 element array of atropy Quantities
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
waverange : 2 element array of atropy Quantities
waverange : 2 element array of Astropy Quantities

@karllark karllark merged commit f454422 into master Mar 2, 2022
@karllark karllark deleted the add_rebin branch November 24, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants