-
Notifications
You must be signed in to change notification settings - Fork 295
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
Improve flexibility of olci level2 reader #2504
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2504 +/- ##
=======================================
Coverage 94.84% 94.85%
=======================================
Files 337 337
Lines 49571 49617 +46
=======================================
+ Hits 47016 47062 +46
Misses 2555 2555
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Pull Request Test Coverage Report for Build 5210366489
💛 - Coveralls |
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.
This is also adds the ability to specify mask items right? Otherwise, looks good from what I understand.
standard_name: algal_pigment_concentration | ||
units: "lg(re mg.m-3)" | ||
standard_name: algal_pigment_concentration | ||
units: "lg(re mg.m-3)" |
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.
Doesn't this remove the calibration: "reflectance"
from the DataID and .attrs
?
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 that a problem if it does? It's not reflectance data.
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.
Not really, but I'm just pointing out that it did exist, now it doesn't. Since multiple calibrations don't exist I guess it doesn't matter.
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.
yes it is removed, but it is A Good Thing (r) because it never was reflectance data.
Yes, I mentioned it in the description of the PR :) |
This PR adds some flexibility to the olci l2 reader. In particular, it adds the the possibility to un-log10 the data which has units like
lg(re mg.m-3)
and lets the user provide its own mask items.