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

Refactor Sentinel-1 SAR-C reader #2727

Merged
merged 17 commits into from
Apr 29, 2024
Merged

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Jan 26, 2024

This PR refactors the SAR reader not to depend on the FileYAMLReader.

Depends on pytroll/pyresample#578

  • Tests added

Todo:

  • Restore GCPs

@mraspaud mraspaud added the enhancement code enhancements, features, improvements label Jan 26, 2024
@mraspaud mraspaud self-assigned this Jan 26, 2024
@mraspaud
Copy link
Member Author

mraspaud commented Mar 8, 2024

pre-commit.ci autofix

@mraspaud mraspaud marked this pull request as ready for review March 8, 2024 09:25
@mraspaud mraspaud requested a review from djhoese as a code owner March 8, 2024 09:25
@pnuu pnuu changed the title Refactor sar reader Refactor Sentinel-1 SAR-C reader Mar 8, 2024
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

Some suggestions and comments inline.

Comment on lines +736 to +737
def create_storage_items(self, files, **kwargs):
"""Create the storage items."""
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure I understand what happens here, but it looks more like file handler creation rather than collecting the stored data. Maybe naming and docstring could be updated/expanded a bit? What are the "items"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea here was to have a function name that would be more generic than filehandlers. Hence the renaming to storage items.
In the case of this reader, with still have a loose usage of filehandlers, but in principle a dev implementing a new reader would use this function just to initialise storage "items", or "units" or "objects" from the filenames.

Comment on lines +704 to +712
@property
def start_time(self):
"""Get the start time."""
return self.storage_items.values()[0].filename_info["start_time"].replace(tzinfo=tz.utc)

@property
def end_time(self):
"""Get the end time."""
return self.storage_items.values()[0].filename_info["end_time"].replace(tzinfo=tz.utc)
Copy link
Member

Choose a reason for hiding this comment

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

These look a bit costly (maybe, see my comment on storage item collection), should these be cached properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this, as in theory the reader object is not immutable... a cached property would make the assumption that the files/storage items never changes.
I know that in practice, this never happens, but this may still be confusing and bug-prone

Copy link
Member

Choose a reason for hiding this comment

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

Is this different to other (traditional) readers and one can append more data to it? Or what would be the mechanism that changed the times after Scene creation?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it's the same compared from the reader level (not the filehandler). In theory, nothing prevents a user to call create_filehandlers (now create_storage_items) multiple times on the same reader...

satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 97.53086% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 95.94%. Comparing base (12854de) to head (f0c382e).
Report is 189 commits behind head on main.

❗ Current head f0c382e differs from pull request most recent head 3b40675. Consider uploading reports for the commit 3b40675 to get more accurate results

Files Patch % Lines
satpy/readers/sar_c_safe.py 97.01% 4 Missing ⚠️
satpy/readers/yaml_reader.py 95.06% 4 Missing ⚠️
satpy/tests/reader_tests/test_sar_c_safe.py 98.85% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2727    +/-   ##
========================================
  Coverage   95.93%   95.94%            
========================================
  Files         375      375            
  Lines       53237    53456   +219     
========================================
+ Hits        51075    51290   +215     
- Misses       2162     2166     +4     
Flag Coverage Δ
behaviourtests 4.11% <2.96%> (-0.02%) ⬇️
unittests 96.04% <97.53%> (+<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

coveralls commented Mar 8, 2024

Pull Request Test Coverage Report for Build 8879886090

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-96.0%) to 0.0%

Totals Coverage Status
Change from base Build 8200761354: -96.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@mraspaud
Copy link
Member Author

@pnuu @djhoese anything against merging this?

@pnuu
Copy link
Member

pnuu commented Apr 17, 2024

Merging is fine from my point of view.

@mraspaud mraspaud added this to the v0.48.0 milestone Apr 17, 2024
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

I have a couple requests, but otherwise I just wanted to say it is unfortunate that all that base reader refactoring had to happen in this PR with all these other changes. Do any of the reader documentation pages need to be updated to reflect the separate classes like the custom reader instructions?

satpy/readers/__init__.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Show resolved Hide resolved
satpy/readers/sar_c_safe.py Outdated Show resolved Hide resolved
satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
satpy/readers/sar_c_safe.py Show resolved Hide resolved
@mraspaud
Copy link
Member Author

I have a couple requests, but otherwise I just wanted to say it is unfortunate that all that base reader refactoring had to happen in this PR with all these other changes.

In my defense, I didn't see this as a major refactor, as I just split a class into two without any changes to the code iirc, and renamed create_filehandlers to create_storage_items. Sorry that I made a mess.

Do any of the reader documentation pages need to be updated to reflect the separate classes like the custom reader instructions?

We don't need to update any documentation now I think, as we (or I) am still looking for the best way to implement that kind of reader (ie reader with many files depending on each other). But in the future, we indeed need to improve the documentation to show how to create a custom reader this way.

@djhoese
Copy link
Member

djhoese commented Apr 29, 2024

In my defense, I didn't see this as a major refactor, as I just split a class into two without any changes to the code iirc, and renamed create_filehandlers to create_storage_items. Sorry that I made a mess.

I think the create_filehandlers change is the most jarring as it is technically public API that is being changed. It is only a rename right? I can live with it, but some users may complain if they're doing something custom.

@mraspaud
Copy link
Member Author

I think the create_filehandlers change is the most jarring as it is technically public API that is being changed. It is only a rename right? I can live with it, but some users may complain if they're doing something custom.

to be clearcreate_filehandlers is still here, I just added create_storage_items as an alternative. So nothing custom should break.

@djhoese
Copy link
Member

djhoese commented Apr 29, 2024

Ah great. Sounds good then.

@mraspaud mraspaud modified the milestones: v0.48.0, v0.49.0 Apr 29, 2024
Comment on lines +720 to +734
for key in dataset_keys:
for handler in self.storage_items.values():
val = handler.get_dataset(key, info=dict())
if val is not None:
val.attrs["start_time"] = handler.start_time
if key["name"] not in ["longitude", "latitude"]:
lonlats = self.load([DataID(self._id_keys, name="longitude", polarization=key["polarization"]),
DataID(self._id_keys, name="latitude", polarization=key["polarization"])])
gcps = val.coords["spatial_ref"].attrs["gcps"]
from pyresample.future.geometry import SwathDefinition
val.attrs["area"] = SwathDefinition(lonlats["longitude"], lonlats["latitude"],
attrs=dict(gcps=gcps))
datasets[key] = val
continue
return datasets
Copy link
Member

Choose a reason for hiding this comment

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

I'll merge this PR because it fixes the CI for everyone else, but it'd be nice to have a followup PR to fix CodeScene issues like this method being too complex.

@djhoese djhoese merged commit a47b439 into pytroll:main Apr 29, 2024
15 of 17 checks passed
@mraspaud mraspaud deleted the sar-independent-reader branch April 30, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants