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 more realistic METimage RSRs #109

Merged
merged 3 commits into from
May 14, 2020

Conversation

rolle-at-work
Copy link
Contributor

@rolle-at-work rolle-at-work commented May 11, 2020

Updated to more realistic, though not measured spectral response functions for METimage

  • Closes #xxxx
  • Tests added
  • Tests passed: Passes pytest pyspectral
  • Passes flake8 pyspectral
  • Fully documented

@coveralls
Copy link

coveralls commented May 11, 2020

Coverage Status

Coverage remained the same at 71.113% when pulling 1cf912c on rolle-at-work:add_simulated_metimage_rsr into 0f6f145 on pytroll:master.

@rolle-at-work
Copy link
Contributor Author

The new file looks good and i tested reading with hdfview and a pyspectral plotting routine.

@adybbroe adybbroe self-assigned this May 12, 2020
@adybbroe adybbroe self-requested a review May 12, 2020 13:54
Copy link
Collaborator

@adybbroe adybbroe left a comment

Choose a reason for hiding this comment

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

@rolle-at-work I think you could/should use the todf5 function in utils.py. Then we will also get rid of the redundant det-1 layer. There is only one detector, and in that case I think we should not use that extra layer

@rolle-at-work
Copy link
Contributor Author

@adybbroe But is this layer also gone for AVHRR and other one-detector instruments? I think this layer was usefull to allow for a general reader of bof both single and multi detector instruments.

@adybbroe
Copy link
Collaborator

Yes @rolle-at-work they are gone for all single detector sensors. They should be at least. I just checked a couple. And the reader can handle both, no problem

@adybbroe
Copy link
Collaborator

Oh well, now I look back in the history here:
#68
Seems like we were preparing for more detectors. So, if we know there will be, and I gues we do know that @rolle-at-work? Then we should keep it as is I think. Sorry

@rolle-at-work
Copy link
Contributor Author

Yes @adybbroe METimage will have 24 detectors per channel

@adybbroe
Copy link
Collaborator

@rolle-at-work I have uploaded an updated RSR data archive with your new Metimage data included, please check it out and adapt your PR accordingly. You will need to change version and url of the RSR data in utils.py Version should now be RSR_DATA_VERSION = "v1.0.15" and the url is now https://zenodo.org/record/3824535/files/pyspectral_rsr_data.tgz

Copy link
Collaborator

@adybbroe adybbroe left a comment

Choose a reason for hiding this comment

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

So, @rolle-at-work, could you please update utils.py according to my comments

Copy link
Collaborator

@adybbroe adybbroe left a comment

Choose a reason for hiding this comment

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

LGTM

@adybbroe adybbroe marked this pull request as ready for review May 14, 2020 09:47
@adybbroe adybbroe merged commit 7d30889 into pytroll:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants