-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix rsr plotting to allow for similar (overlapping) bands #240
Fix rsr plotting to allow for similar (overlapping) bands #240
Conversation
…ers than the first found Signed-off-by: Adam.Dybbroe <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #240 +/- ##
=======================================
Coverage 90.25% 90.25%
=======================================
Files 22 22
Lines 2515 2515
=======================================
Hits 2270 2270
Misses 245 245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Maybe it's just me but those quicklooks aren't working. They still say "Uploading ...png...". Did you maybe close the tab before they were fully uploaded? |
@@ -103,6 +103,11 @@ def get_arguments(): | |||
help="The wavelength range for the plot", | |||
default=[None, None], type=float) | |||
|
|||
parser.add_argument("--exclude_bandnames", nargs='*', | |||
default=[], | |||
required=False, |
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.
required
is not necessary here. The default is False
. Actually type=str
is also not necessary as str
is the default type.
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.
Small change requested. I don't understand the code before or after this PR so I'm not sure I should give my opinion too strongly. The code "smells" like there should be a simpler way, but if it works for you then 👍.
Possible simplification: Could you have a function that gets all the bands needed to be plotted and would take the wavelength and band information in as input and produce a list of bands?
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.
LGTM! I'm not expert enough to figure out how to do what Dave asks but they appear like reasonable suggestions if you have time. If not, happy to see it merged as-is.
You were right, sorry, should work now |
@adybbroe Can you fix the title of the PR. It got cut off from your commit message it seems. For your quicklooks, yes I see them, but now I'm not sure what you are trying to show. Prior to this PR the code would just ignore I5? Or would it fail because two wavelengths matched for a single VIIRS instrument? And now? Now you don't need the exclude I5 because it can be plotted without issue? |
I believe you have a sharp nose! I think you might be right. Will think another day on this, and either start simplify, or merge and make an issue, or just merge :-) |
Fix the script to plot spectral response functions, allowing for similar (overlapping) bands - and don't just ignore all others than the first found.
pytest pyspectral
flake8 pyspectral
AUTHORS.md
if not there already