Refactor Scene loading and dependency tree #1351
Conversation
mraspaud
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #1351 +/- ##
==========================================
+ Coverage 90.58% 91.62% +1.03%
==========================================
Files 239 246 +7
Lines 34313 36015 +1702
==========================================
+ Hits 31081 32997 +1916
+ Misses 3232 3018 -214
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
satpy/scene.py
Outdated
| ".mitiff": "mitiff"} | ||
| return mapping.get(extension.lower(), 'simple_image') | ||
|
|
||
| def remove_failed_datasets(self, keepables): |
There was a problem hiding this comment.
Did this have to be made public for the case where we call it above on new_scn?
There was a problem hiding this comment.
yes, exactly. As you know, it's bad practice to use the private methods of a class outside of it.
|
The refactoring here is a WIP (hence the draft). I'm trying to see if I can get the channel and composite loading logic out of the |
This fixes pytroll#1443.
There was a problem hiding this comment.
GitHub made this really hard to review as it didn't really understand the rearrangement of Scene methods. I'm not sure I fully understand everything that was done, but I know a lot of the complexity of the dep tree is covered by the tests so I'm not afraid to merge this.
However, you removed property from missing_datasets which is a significant change for a refactor PR, especially since it doesn't seem like it is needed (it doesn't take any additional arguments). I also had some questions on some of the variable names you chose.
satpy/scene.py
Outdated
| ds = xr.Dataset(ds_dict, coords={"latitude": (["y", "x"], lats), | ||
| "longitude": (["y", "x"], lons)}) | ||
|
|
||
| ds.attrs = mdata | ||
| return ds | ||
|
|
||
| def _get_actual_dataarrays(self, datasets): |
There was a problem hiding this comment.
Is there a better name for this?
There was a problem hiding this comment.
replaced with _get_dataarrays_from_identifiers
satpy/dependency_tree.py
Outdated
|
|
||
| def _find_matching_ids_in_readers(self, dataset_key): | ||
| matching_ids = {} | ||
| for reader_name, reader_instance in self.readers.items(): | ||
| matching_ids[reader_name] = [] | ||
| for file_handler_name, reader_instance in self.readers.items(): |
There was a problem hiding this comment.
Indeed. I got confused by looking at the values, forgetting that the reader name is also set in the yaml file for the format. Reverted.
satpy/dependency_tree.py
Outdated
| @@ -343,13 +339,13 @@ def _get_unique_id_from_sorted_ids(sorted_ids, distances): | |||
| raise MissingDependencies | |||
| return result | |||
|
|
|||
| def _get_unique_node_from_id(self, result, data): | |||
| def _get_unique_reader_node_from_id(self, unique_id, reader_name): | |||
There was a problem hiding this comment.
unique_id? Why not call it data_id if that's what it is?
satpy/scene.py
Outdated
| @@ -217,7 +217,6 @@ def end_time(self): | |||
| """Return the end time of the file.""" | |||
| return self.attrs['end_time'] | |||
|
|
|||
| @property | |||
| def missing_datasets(self): | |||
There was a problem hiding this comment.
Why is this no longer a property? This is a backwards incompatibility. Not really the best thing for a refactor PR.
There was a problem hiding this comment.
I agree this shouldn't be in a refactor PR, I will remove it. The PR was hastily finished, and that needed more work obviously which hasn't been done yet.
However, the reason I have this here is that I believe it should be a function, not a property. I got properly confused when doing the refactoring because I wasn't expecting the value of missing_dataset to be changing as it did between load calls. Maybe that's just me, but it seemed much more appropriate to make this a function then.
There was a problem hiding this comment.
because I wasn't expecting the value of missing_dataset to be changing as it did between load calls
Not sure I understand how making it a method stops it from changing between load calls. Also not sure why it wouldn't change between load calls. If you try to load something and it fails, it is missing, if you try to load something else and it fails, it is also missing.
There was a problem hiding this comment.
making it a method doesn't stop it from changing of course, but a method feels more dynamic as opposed to a property or attribute. Anyway this is changed back now.
djhoese
changed the title
This is a start for refactoring the loading of data in the scene class.
More changes will be needed when a way forward emerges from the discussion in #1469
This PR also solves #1443.
flake8 satpy