-
Notifications
You must be signed in to change notification settings - Fork 293
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 CFWriter utility into CF directory #2524
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
==========================================
+ Coverage 95.21% 95.26% +0.05%
==========================================
Files 356 367 +11
Lines 51605 51685 +80
==========================================
+ Hits 49134 49239 +105
+ Misses 2471 2446 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 7020542918
💛 - Coveralls |
Here it is a first refactor. The Decisions to be taken:
|
I vote for
the former maybe?
I don't think we need to explicitely test cf methods as long as the save_datasets exercises the code we are interested in.
Why not. One solution would be to have a base test class that is then inherited by |
Hey @djhoese :) In case you manage to get a quick look in the coming days to this PR, next Tuesday/Wednesday I should have time to address your comments ;) Since we discussed in a previous PR to add options like |
I'm fine with kwarg changes being moved to another PR. I haven't reviewed this in-depth because of Martin's comments, but I now realize most of the comments here are your own about decisions that need to be made. I'll see what I can do. |
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.
In perspective of a ZarrWriter implementation, we aim to run test_cf.py tests twice (for writer='cf' (or writer='netcdf') and writer='zarr')?
My preference would be to have tests that actually test what the writers are doing and what they are responsible for. The CF tests should test that the proper conversions are happening. The writer tests don't need to make sure that CF conversion for every case happened because the CF tests should have covered that.
In the cases where you want to make sure that something the CF conversion is doing is saved/rendered properly on-disk then I think putting it in a writer test is fine, but you could also use to_netcdf
and to_zarr
directly...maybe. I'm not sure how much I'll regret saying that later.
Most of this looks fine to me. I left some inline comments trying to address some of the questions you had. I think in general if you think some functions could be extracted and/or shared then go for it.
I think I requested this somewhere else, but I'd really like if we could be more specific with function names and module names regarding what is private (_
prefix) and public. For example, if a module in satpy/writers/cf/
is only used internally to that sub-package/module then I think the module name could start with a _
. Similarly, if a function is only used inside its own module then it should get a _
prefix to make sure users/readers don't assume it can be imported into somewhere else. The opposite of this is that you shouldn't be importing a function with a _
from one module to another. Thoughts?
The readthedocs failure seems like a corrupt download. It should fix itself next run. |
Nevermind:
|
@djhoese I further simplified all functions that CodeScene was complaining about. Have a last look ... in case tomorrow I can do the last changes ;) |
@mraspaud do you want to give this one last review? |
Maybe also @TomLav want to have a look in light of pytroll/pyresample#278 ? |
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.
Thanks for putting this together, looks good to me.
A question about
For For In pyresample's For I have not looked at the test suite yet, but do you have tests that use CFWriter to dump an area_def, then re-read it with |
It should be noted that the angles for the CF geostationary case are satellite viewing angles in radians NOT lon/lat degrees. Regardless a NetCDF is still valid with metered coordinates for geos as long as the units attribute states that. |
The units of the
So the validity of satpy using |
@TomLav I'm a little confused by that. It seems like they are providing part of the explanation and not the whole thing. I also noticed that canonical units are defined as:
So obviously meters and kilometers are "physically equivalent", but based on the quote you commented they're saying radians and meters are not. So if we're using the CF 1.9 specification and producing a geostationary grid mapping then we should (only) produce x/y variables with standard_name |
@ghiggi I see that the directory now is |
@mraspaud I don't recall precisely. Maybe we had a discussion with @djhoese on this ... I don't remember. |
I think I like the idea of a generic |
Merged! |
AUTHORS.md
if not there alreadyRefactor CFWriter-related functions into separate CF modules.