- Notifications
You must be signed in to change notification settings - Fork313
A new reader for the netcdf sent to Icare by Meteo-France.#2911
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pre-commit.ci autofix |
for more information, seehttps://pre-commit.ci
@lperier thanks for adding this reader! As you can see, the CI is complaining about a few things, that would be nice to fix as many as possible. Another important point is adding tests for this new reader. The way we do it nowadays is to generate a data file in a temporary directory (using pytest's |
@lperier the error that is now in the tests is because of an external issue. If you merge your branch with main, it should disappear. |
Pytest program to test the reader geos_netcdficare.py.
codecovbot commentedOct 15, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2911 +/- ##==========================================- Coverage 96.06% 96.01% -0.05%========================================== Files 373 379 +6 Lines 54465 55813 +1348 ==========================================+ Hits 52321 53589 +1268- Misses 2144 2224 +80
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I added a few comments inline.
In general, try to follow the python coding guidelines. There are some more coding guidelines here:https://pytroll.github.io/guidelines.html
If you install ruff, it will help you spot style issues in your code also.
satpy/readers/geos_netcdficare.py Outdated
| # chunk_bytes = self._chunk_bytes_for_resolution() | ||
| chunk_bytes = '128 MiB' | ||
| # chunk_bytes = '64 MiB' | ||
| # with dask.config.set(**{'array.slicing.split_large_chunks': True}) : | ||
| with dask.config.set({"array.chunk-size": chunk_bytes}) : |
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 should be set by the user outside of satpy, so this code needs to be removed.
satpy/readers/geos_netcdficare.py Outdated
| self.metadata["end_time"] = self.get_endOrStartTime( | ||
| "time_coverage_end") | ||
| print( |
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.
Satpy uses logging, not printing... see other readers.
satpy/readers/geos_netcdficare.py Outdated
| self.longitudeReelle = self.satlon() | ||
| self.longitudedeprojection = self.projlon() |
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.
Please use variable names in english, and snake_case, not CamelCase :)
satpy/readers/geos_netcdficare.py Outdated
| -------- | ||
| Here is an example how to read the data in satpy: | ||
| from satpy import Scene |
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.
Satpy uses 4 space indentations, not tabs
satpy/readers/geos_netcdficare.py Outdated
| # from netCDF4 import Dataset | ||
| # from xarray import DataArray |
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.
Should be remove if not needed
satpy/readers/geos_netcdficare.py Outdated
| # from satpy._compat import cached_property | ||
| # from satpy.utils import get_dask_chunk_size_in_bytes |
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.
should be removed if not needed
satpy/readers/geos_netcdficare.py Outdated
| # from satpy.utils import get_dask_chunk_size_in_bytes | ||
| import dask | ||
| from satpy.readers import open_file_or_filename | ||
| # import math |
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.
Should be removed if not needed.
satpy/readers/geos_netcdficare.py Outdated
| # Reading the needed attributes. | ||
| self.initialisation_dataset() | ||
| # __init__() |
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.
Remove.
satpy/readers/geos_netcdficare.py Outdated
| print( | ||
| "Unsupported satellite platform : " + | ||
| self.plateforme) | ||
| exit(1) |
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.
Please use exceptions, not exitting.
satpy/readers/geos_netcdficare.py Outdated
| self.plateforme) | ||
| exit(1) | ||
| # sensor_name() |
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 function should return the sensor name. Also, we try to avoid setting attributes in methods other than__init__
AUTHORS.mdif not there already