Add pass through of xr compute, persist and chunk to Scene #1017

BENR0 · 2019-12-11T15:49:55Z

Adds the xarray interfaces compute, persist and chunk to Scene.

If for example scn.compute() is called it is iterated over all Datasets in the Scene and compute is called on every Dataset (which is a xarray.DataArray).

Closes Add compute method to Scene #1015
Tests added and test suite added to parent suite
Tests passed
Passes flake8 satpy
Fully documented
Add your name to AUTHORS.md if not there already

djhoese · 2019-12-11T16:05:58Z

Nice start. I just thought of something, in order to match the xarray/dask interfaces these should probably return a new Scene object. Thoughts?

coveralls · 2019-12-11T16:14:42Z

Coverage decreased (-0.03%) to 87.334% when pulling b4226b9 on BENR0:xarray_interfaces into fb61664 on pytroll:master.

codecov · 2019-12-11T16:14:56Z

Codecov Report

Merging #1017 into master will decrease coverage by 0.03%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
- Coverage   87.36%   87.33%   -0.04%     
==========================================
  Files         183      183              
  Lines       28161    28194      +33     
==========================================
+ Hits        24603    24623      +20     
- Misses       3558     3571      +13

Impacted Files	Coverage Δ
satpy/scene.py	`88.69% <20%> (-1.67%)`	⬇️
satpy/writers/cf_writer.py	`90.67% <0%> (-0.42%)`	⬇️
satpy/tests/writer_tests/test_cf.py	`98.49% <0%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb61664...8281c57. Read the comment docs.

BENR0 · 2019-12-12T12:34:25Z

Haven't thought about that it's a good point. Yes I think if xarray and dask return new objects satpy should too. Maybe we can make that a default but add a parameter like "inplace"?

djhoese · 2019-12-12T14:02:01Z

Looks like stickler isn't so happy with your indentation. I'm not sure I like the inplace kwarg, does it really provide you anything? Pandas, xarray, and dask are all not inplace anymore. Although I understand the _apply decorator, I'm worried it causes a little too much indirection. I'm willing to be convinced, but just worried at first glance.

BENR0 · 2019-12-12T15:19:15Z

Actually after adding the scn.copy() part I was not so convinced myself since in any way a Scene is returned so from my side we can remove the "inplace" parameter.
It's kind of funny I wasn't sure of the decorator either because the saving of repetitive code is minimal. So I guess you are thinking of moving it into the scene and just call it in each return?

djhoese · 2019-12-12T15:27:59Z

So I guess you are thinking of moving it into the scene and just call it in each return?

What is "it" in that question? I was thinking doing a new_scn = scn.copy() followed by the for loop with the compute/persist/chunk calls in each method was good enough.

BENR0 · 2019-12-12T15:55:40Z

I meant the apply function with "it". That would at least save some code. But sure we can also copy the loop to every method. I will change that later or tomorrow morning then.

djhoese

Great job. Looks nice and clean. Do you think you could add some tests?

Otherwise, I wonder if you could link to the xarray method in the docstrings by doing xarray.DataArray.chunk when referencing the methods (include single backticks around them). I think that sphinx (the intersphinx extension) should pick up on this when rendering the sphinx docs. If it doesn't then maybe you could add a second line to the docstring like:

See :meth:`xarray.DataArray.chunk` for more details.

Lastly, method docstrings should end in a period. Do you think you could add those? We should probably make stickler start checking docstrings.

codecov · 2019-12-13T17:01:57Z

Codecov Report

Merging #1017 (d3bdfdc) into main (b587681) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
+ Coverage   93.39%   93.47%   +0.07%     
==========================================
  Files         273      275       +2     
  Lines       40612    40772     +160     
==========================================
+ Hits        37929    38111     +182     
+ Misses       2683     2661      -22

Flag	Coverage Δ
behaviourtests	`4.84% <8.10%> (+<0.01%)`	⬆️
unittests	`94.01% <100.00%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/scene.py	`93.02% <100.00%> (+0.20%)`	⬆️
satpy/tests/test_scene.py	`99.45% <100.00%> (+0.01%)`	⬆️
satpy/resample.py	`79.34% <0.00%> (-0.69%)`	⬇️
satpy/readers/seviri_l1b_native.py	`85.39% <0.00%> (-0.25%)`	⬇️
satpy/modifiers/geometry.py	`87.30% <0.00%> (-0.20%)`	⬇️
satpy/readers/ahi_hsd.py	`97.25% <0.00%> (-0.05%)`	⬇️
satpy/readers/fci_l1c_nc.py	`97.93% <0.00%> (-0.05%)`	⬇️
satpy/readers/utils.py	`91.79% <0.00%> (ø)`
satpy/composites/viirs.py	`86.40% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b587681...d3bdfdc. Read the comment docs.

djhoese · 2019-12-13T17:52:11Z

I rendered the sphinx locally for your changes and noticed there needs to be a blank line between the "subject" and the rest of the docstring otherwise they get rendered as one line in the HTML. I also tweaked the verb tense to make flake8-docstring happy. This then lead to flake8-docstring complaining that we were using the name of the method inside the method's docstring. I told flake8 to ignore that for now. @mraspaud thoughts?

I'll see if I can cleanly ignore the check just for those methods instead of globally.

Edit: Got it!

satpy/tests/test_scene.py

stickler-ci · 2020-12-31T14:45:46Z

satpy/scene.py

+        for k in new_scn.datasets.keys():
+            new_scn[k] = new_scn[k].chunk(**kwargs)
+        return new_scn
+


W293 blank line contains whitespace

stickler-ci · 2020-12-31T14:45:46Z

satpy/tests/reader_tests/test_ahi_hrit.py

+        lines_sparse = np.array(list(range(1, nlines, 20)) + [nlines])
+        times_sparse = mjd_1970 + lines_sparse / 24 / 3600
+        acq_time_s = ['LINE:={}\rTIME:={:.6f}\r'.format(l, t)
+                      for l, t in zip(lines_sparse, times_sparse)]


E741 ambiguous variable name 'l'

ghost · 2020-12-31T14:46:44Z

DeepCode's analysis on #fe1a32 found:

⚠️ 1 warning, ℹ️ 33 minor issues. 👇
✔️ 25 issues were fixed.

Top issues

Description	Example fixes
Defining only __eq__ but not __ne__ will result in a Python2 error if objects are compared with inequality. Occurrences: dataid.py:485	🔧 Example fixes
Unused CRS imported from pyproj Occurrences: test_scene.py:507 test_cf.py:957 nucaps.py:36	🔧 Example fixes
Statement seems to have no effect Occurrences: anc_vars.py:31	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

sfinkens

Nice work, very useful!

djhoese · 2021-12-02T15:40:47Z

satpy/scene.py

+        """
+        new_scn = self.copy()
+        for k in new_scn._datasets.keys():
+            new_scn[k] = new_scn[k].compute(**kwargs)


It would be nice if these methods could compute (same for the persist) all the DataArrays at the same time. As is this will likely recompute share dependencies of the DataArrays.

I think I don't exactly understand what you mean with "at the same time".

Normally for dask arrays you want to call res1, res2, res3 = dask.array.compute(array1, array2, array3) so that all dependency calculations for generating those three arrays are only computed once. I'm not sure how that can be done with xarray. You could try passing the DataArrays to da.compute and see if that works.

~~Ah I see. Yes indeed that would be useful. I will check if that is possible with xarray or as you suggested with da.compute.~~

@djhoese I changed the code to use dask.compute / dask.persist now.

djhoese · 2021-12-02T15:41:33Z

I'm not sure what codefactors problem is. If you merged with main then this isn't supposed to be an issue (the asserts in the test directory).

BENR0 · 2021-12-02T16:30:27Z

I'm not sure what codefactors problem is. If you merged with main then this isn't supposed to be an issue (the asserts in the test directory).

There were so many changes since the original PR that it was easier to redo the changes on the current main. That's why I force pushed.

djhoese

Two small suggestions, but otherwise looks good.

djhoese · 2021-12-07T15:25:57Z

satpy/scene.py

@@ -1143,6 +1143,45 @@ def save_datasets(self, writer=None, filename=None, datasets=None, compute=True,
                                          **kwargs)
        return writer.save_datasets(dataarrays, compute=compute, **save_kwargs)

+    def compute(self, **kwargs):
+        """Call `compute` on all Scene datasets.


Should we say "data arrays" here instead of datasets to avoid the future confusion when Scene is more dependent on xarray Dataset objects?

Yes definitely. I think this is better since this otherwise adds to confusion.

satpy/scene.py

Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

…y_interfaces

djhoese · 2021-12-07T17:06:19Z

Looks like the jobs got hung up, I've restarted them.

djhoese

LGTM. @mraspaud or others, have any comments?

mraspaud

LGTM

BENR0 requested review from djhoese and mraspaud as code owners December 11, 2019 15:49

djhoese reviewed Dec 13, 2019

View reviewed changes

mraspaud assigned BENR0 Jan 21, 2020

mraspaud added the enhancement code enhancements, features, improvements label Jan 21, 2020

stickler-ci reviewed Mar 12, 2020

View reviewed changes

BENR0 force-pushed the xarray_interfaces branch from 2d895fd to ae4680f Compare March 12, 2020 14:24

BENR0 requested review from adybbroe, pnuu and sfinkens as code owners December 31, 2020 14:45

stickler-ci reviewed Dec 31, 2020

View reviewed changes

Add xarray compute, persist, chunk pass through

b87eaef

BENR0 force-pushed the xarray_interfaces branch from fe1a326 to b87eaef Compare December 2, 2021 09:04

Add tests

f3441af

sfinkens approved these changes Dec 2, 2021

View reviewed changes

djhoese reviewed Dec 2, 2021

View reviewed changes

refactor: switch from loop compute/persist to dask.compute/persist

9ca4f61

djhoese requested changes Dec 7, 2021

View reviewed changes

BENR0 and others added 3 commits December 7, 2021 17:26

doc: improve precision.

db9c75d

Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

doc: change wording

457b296

Merge branch 'xarray_interfaces' of github.com:BENR0/satpy into xarra…

d3bdfdc

…y_interfaces

djhoese approved these changes Dec 8, 2021

View reviewed changes

mraspaud approved these changes May 3, 2022

View reviewed changes

djhoese added this to In progress in PCW Spring 2022 via automation May 3, 2022

djhoese merged commit e5a71d5 into pytroll:main May 3, 2022

PCW Spring 2022 automation moved this from In progress to Done May 3, 2022

BENR0 deleted the xarray_interfaces branch August 23, 2022 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pass through of xr compute, persist and chunk to Scene #1017

Add pass through of xr compute, persist and chunk to Scene #1017

BENR0 commented Dec 11, 2019 •

edited

djhoese commented Dec 11, 2019

coveralls commented Dec 11, 2019 •

edited

codecov bot commented Dec 11, 2019 •

edited

BENR0 commented Dec 12, 2019

djhoese commented Dec 12, 2019

BENR0 commented Dec 12, 2019 •

edited

djhoese commented Dec 12, 2019

BENR0 commented Dec 12, 2019 •

edited

djhoese left a comment •

edited

codecov bot commented Dec 13, 2019 •

edited

djhoese commented Dec 13, 2019 •

edited

stickler-ci Dec 31, 2020

stickler-ci Dec 31, 2020

ghost commented Dec 31, 2020

sfinkens left a comment

djhoese Dec 2, 2021

BENR0 Dec 2, 2021

djhoese Dec 2, 2021

BENR0 Dec 2, 2021 •

edited

djhoese commented Dec 2, 2021

BENR0 commented Dec 2, 2021

djhoese left a comment

djhoese Dec 7, 2021

BENR0 Dec 7, 2021

djhoese commented Dec 7, 2021

djhoese left a comment

mraspaud left a comment

Add pass through of xr compute, persist and chunk to Scene #1017

Add pass through of xr compute, persist and chunk to Scene #1017

Conversation

BENR0 commented Dec 11, 2019 • edited

djhoese commented Dec 11, 2019

coveralls commented Dec 11, 2019 • edited

codecov bot commented Dec 11, 2019 • edited

Codecov Report

BENR0 commented Dec 12, 2019

djhoese commented Dec 12, 2019

BENR0 commented Dec 12, 2019 • edited

djhoese commented Dec 12, 2019

BENR0 commented Dec 12, 2019 • edited

djhoese left a comment • edited

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2019 • edited

Codecov Report

djhoese commented Dec 13, 2019 • edited

stickler-ci Dec 31, 2020

Choose a reason for hiding this comment

stickler-ci Dec 31, 2020

Choose a reason for hiding this comment

ghost commented Dec 31, 2020

DeepCode's analysis on #fe1a32 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

sfinkens left a comment

Choose a reason for hiding this comment

djhoese Dec 2, 2021

Choose a reason for hiding this comment

BENR0 Dec 2, 2021

Choose a reason for hiding this comment

djhoese Dec 2, 2021

Choose a reason for hiding this comment

BENR0 Dec 2, 2021 • edited

Choose a reason for hiding this comment

djhoese commented Dec 2, 2021

BENR0 commented Dec 2, 2021

djhoese left a comment

Choose a reason for hiding this comment

djhoese Dec 7, 2021

Choose a reason for hiding this comment

BENR0 Dec 7, 2021

Choose a reason for hiding this comment

djhoese commented Dec 7, 2021

djhoese left a comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

BENR0 commented Dec 11, 2019 •

edited

coveralls commented Dec 11, 2019 •

edited

codecov bot commented Dec 11, 2019 •

edited

BENR0 commented Dec 12, 2019 •

edited

BENR0 commented Dec 12, 2019 •

edited

djhoese left a comment •

edited

codecov bot commented Dec 13, 2019 •

edited

djhoese commented Dec 13, 2019 •

edited

BENR0 Dec 2, 2021 •

edited