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

Remove slstr_l2 reader in favor of ghrsst_l2 #2731

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Jan 29, 2024

This fixes a long time broken reader. As discussed by @ameraner in #2319, @adybbroe made a PR renaming/refactoring that included moving the slstr_l2 reader python code to the ghrsst_l2 reader, but that PR forgot to remove/rename/deprecate the YAML file. This PR finally removes that and adds the reader name to the dictionary of deprecated readers.

I'm not sure I agree with the "equivalence" of the two readers since the variables were renamed to be, for example, sea_surface_temperature_slstr. I'm not familiar with these readers and data files, but wouldn't it be more user friendly to share the Python code, but have names without the instrument suffix on the variables?

CC @simonrp84 who originally brought this to my attention.

@djhoese djhoese added bug component:readers backwards-incompatibility Causes backwards incompatibility or introduces a deprecation labels Jan 29, 2024
@djhoese djhoese self-assigned this Jan 29, 2024
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4be3e17) 95.40% compared to head (07d258d) 95.41%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2731   +/-   ##
=======================================
  Coverage   95.40%   95.41%           
=======================================
  Files         371      371           
  Lines       52825    52825           
=======================================
+ Hits        50399    50402    +3     
+ Misses       2426     2423    -3     
Flag Coverage Δ
behaviourtests 4.15% <100.00%> (ø)
unittests 96.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7702576821

  • 0 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.005%) to 95.973%

Files with Coverage Reduction New Missed Lines %
satpy/tests/test_readers.py 1 99.36%
Totals Coverage Status
Change from base Build 7702571788: 0.005%
Covered Lines: 50528
Relevant Lines: 52648

💛 - Coveralls

@simonrp84
Copy link
Member

Seems fine to me.

For context, I am writing an actual L2 reader for SLSTR (in support of fire products) so I won't make my PR for that until this one is merged.

@djhoese
Copy link
Member Author

djhoese commented Jan 30, 2024

If there are other L2 products that can be supported doesn't that mean that I shouldn't merge this as-is but instead we fix the reference to the python code in this YAML file?

@ameraner
Copy link
Member

ameraner commented Feb 5, 2024

Looks to me like the "formally cleanest" way would be to merge this PR as-is, in order to remove the broken slstr_l2 reader, also considering that fixing the python reference in the slstr_l2.yaml file would only lead to a "SLSTR L2" reader that only accepts one specific L2 product, which is not optimal. After that, Simon can add a proper slstr_l2 reader, possibly by re-using/merging the python parts depending on the formats.

@djhoese
Copy link
Member Author

djhoese commented Feb 5, 2024

Sorry, I need some clarification about what all of this stuff is. SLSTR is the instrument, right? GHRSST is a algorithm/SST group, but is it also a processing/algorithm software package? Are the SST files generated and previously supported by slstr_l2 an output of this GHRSST package? For the other products that @simonrp84 wants to support, are these "official" products? Or are they all outputs of different processing packages and from different algorithm groups and none of them officially supported/recommended by the organization in charge of SLSTR?

It is not uncommon for an L2 reader to exist with support for only some datasets. I don't have a huge problem with that although it is misleading a bit to new users. Even if it takes @simonrp84 6-12 months to add support for other datasets, I'm not that concerned that there will be that many upset users who are surprised by the limited offering of the current version of the reader. Especially since it existed like that for many years before this and was only just discovered to not be functional.

@djhoese
Copy link
Member Author

djhoese commented Feb 6, 2024

As discussed in the monthly status meeting today, the GHRSST files will likely not be similar to any files that @simonrp84 wants to add support for. So it would make sense for the SST files to only be supported in the ghrsst_l2 reader. We also agreed that merging this PR to deprecate/remove the old reader is the best move for now. In the future if/when @simonrp84 re-adds the slstr_l2 reader he can do that, but will need to remember to remove the deprecation in satpy/readers/__init__.py.

@djhoese djhoese merged commit 8a00b73 into pytroll:main Feb 6, 2024
18 of 19 checks passed
@djhoese djhoese deleted the remove-slstr-l2 branch February 6, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatibility Causes backwards incompatibility or introduces a deprecation bug component:readers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slstr_l2.yaml points to deleted slstr_l2.py
4 participants