isospectrum debugging / refactoring #89

apatlpo · 2020-02-08T13:43:38Z

This PR attemps to fix several issues related to isospectrum computations:

breaks with dask arrays (dask bincount does not handle mixed types arguments)
breaks with extra dimensions

See gist for illustration of failures

I also propose a modest refactoring which consists in adding an isotropize methods that average spectra or crossspectra azimuthaly.
This method is called by isotropic_powerspectrum and isotropic_crossspectrum

There are still some improvements that could be made:

isotropize could be made more robust
the present form is not lazy (because dask bincount is not able to predict array size)

If you believe this PR is worth anything, let me know, I can finetune, even though weeks to come are going to be busy with Ocean Sciences and all.

cheers

rabernat · 2020-02-08T15:33:53Z

Hi @apatlpo -- thanks a lot for the PR! I think this makes a lot of sense, both in terms of fixing bugs
("breaks with extra dimensions" is #9, a long standing issue) and code simplicity.

Can you just have a look at the test failure?

apatlpo · 2020-02-08T18:25:25Z

Sounds good, I'll try clean up the PR.

codecov-io · 2020-02-09T19:26:10Z

Codecov Report

Merging #89 into master will increase coverage by 0.38%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   97.08%   97.47%   +0.38%     
==========================================
  Files           2        2              
  Lines         755      752       -3     
  Branches      130      118      -12     
==========================================
  Hits          733      733              
+ Misses         12       10       -2     
+ Partials       10        9       -1

Impacted Files	Coverage Δ
xrft/tests/test_xrft.py	`99.09% <100%> (+0.07%)`	⬆️
xrft/xrft.py	`95.16% <100%> (+0.33%)`	⬆️

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 9de9817...5ed19a6. Read the comment docs.

apatlpo · 2020-02-09T19:26:42Z

I'd like to propose renaming isotropic_powerspectrum and isotropic_crossspectrum.
Here are some suggestions:

isotropic_power_spectrum, isotropic_cross_spectrum
isopower_spectrum, isocross_spectrum

My preference goes to the second option.
What do people think?

apatlpo · 2020-02-09T19:28:35Z

Things to do:

improve test of isotropic_*spectrum
add test for isotropize
try to bullet proof isotropize
specify in isotropize doc and related methods that the code is not lazy anymore
investigate options to keep the code lazy (should be doable)

rabernat · 2020-02-15T19:34:09Z

I'd like to propose renaming isotropic_powerspectrum and isotropic_crossspectrum.
Here are some suggestions:

I'm ok with this, but let's do a deprecation cycle. We will keep the old function names for now but just have them call the new functions and issue a DeprecationWarning. Then in a future release we will remove them completely.

I personally prefer isotropic_power_spectrum and isotropic_cross_spectrum because they are more explicit.

apatlpo · 2020-02-15T21:49:28Z

Sounds good.

I won't be able to tackle before approx 1 week unfortunately.

apatlpo · 2020-03-02T12:29:50Z

this PR has been moving along and is close to be reviewed.

Before then, do I need to address codecov failings?

I am not sure what I'm supposed to do with these.

apatlpo · 2020-03-03T16:48:08Z

I was about to say that this was ready for review but got an issue with python 2.7 test.
Do I really need to install 2.7 and figure what's wrong?

ping @rabernat

roxyboy · 2020-03-03T16:51:12Z

I'm in favor of ending support for py27...

roxyboy · 2020-03-06T11:19:19Z

If we're going to keep support py27 as for now, one way would be to check the python version on the run with if sys.version_info[0] < 3:.

rabernat · 2020-03-06T11:45:59Z

Let's just drop py2.7. We don't have enough users for a deprecation cycle.

@apatlpo - just remove the py2.7 environment from the travis config and let's see if the tests go green.

rabernat

This is looking great. Thanks for all your work @apatlpo!

rabernat · 2020-03-06T11:47:13Z

xrft/tests/test_xrft.py

@@ -652,13 +652,68 @@ def _synthetic_field(N, dL, amp, s):
    theta = np.fft.ifft2(np.fft.ifftshift(F_theta))
    return np.real(theta)

+def _synthetic_field_xr(N, dL, amp, s,


You don't need to use private functions in tests. It's not part of public API anyway. Rename to synthetic_field.

There should also probably be a fixture.

I'm not sure what a fixture precisely is, but I am under the impression that because I am calling the method multiple times with different input parameters it may not be suited to this case. Correct?

rabernat · 2020-03-06T11:49:59Z

xrft/tests/test_xrft.py

+        spacing_tol = 1e-3
+        nfactor = 4
+        ps = xrft.power_spectrum(da, spacing_tol, dim=dims)
+        ps_iso = xrft.isotropize(ps, fftdim, nfactor=nfactor)    


This test is not actually checking anything. It's just verifying that the function does not raise an error.

Here's an idea: rather than calling power_spectrum, can you just make up a synthetic spectrum with a known isotropic spectrum, and then verify that isotropize does the right thing? You will want some assert statements in there somewhere.

rabernat · 2020-03-06T11:51:04Z

xrft/xrft.py

+    Deprecated function. See isotropic_cross_spectrum doc
+    """
+    import warnings
+    msg = "Deprecated. Use isotropic_cross_spectrum instead"


What about "This function has been renamed and will disappear in the future. Please use isotropic_cross_spectrum instead"

apatlpo · 2020-03-06T15:35:56Z

Travis goes through but not codecov.
Let me know if I need to do anything else.

rabernat · 2020-03-07T02:57:35Z

Codecov is telling us that the test coverage has gone slightly down. The coverage tells you which percentage of the code base is covered by tests, so this means there are some new code paths that are not tested. You can use the codecov site to figure out where exactly these are. If you can add some tests

This may seem like a pain, but it's very useful to have high test coverage like we have now, and we want to try to maintain this. You can check coverage locally while developing by running this command:

xrft/.travis.yml

Line 47 in 41cf806

- py.test xrft --cov=xrft --cov-config .coveragerc --cov-report term-missing -v

This should be a relatively simple fix, but it turns out to be complicated or annoying, we can just accept the drop in coverage and move on.

Thanks for your perseverance with this PR.

apatlpo · 2020-03-07T12:58:08Z

thanks for the explanation Ryan.

I had to add tests for the methods that will be soon deprecated.

apatlpo · 2020-03-10T15:09:20Z

Ping @rabernat @roxyboy
All lights are green

rabernat · 2020-03-10T15:55:59Z

I had to add tests for the methods that will be soon deprecated.

Ah ok, I understand. In that case, please add a # pragma: no cover comment (https://coverage.readthedocs.io/en/coverage-4.3.3/excluding.html) to the deprecated methods and then remove the redundant tests.

Then we are done.

apatlpo · 2020-03-10T21:27:32Z

done

rabernat · 2020-03-11T02:34:20Z

Fantastic! Thanks so much for your persistence! We really appreciate having you as a contributor to xrft.

Aurélien Ponte added 2 commits February 8, 2020 14:01

start working on isotropic spectrum computations

e026550

Update xrft.py

bbb37ff

minor debug to pass CI tests

692ed5b

Aurélien Ponte added 6 commits March 1, 2020 21:07

WIP lay ground for moving this PR forward

deaedff

WIP isotropize should be more robust

6ad2764

WIP first pass at tests

b770dff

WIP try to fix 2.7 tests

130fe85

WIP fix division by 0 warning

234bb6f

WIP fix masking of area

1dcd742

Aurélien Ponte added 2 commits March 3, 2020 17:28

major simplification of azimuthal averages

72b9322

fix tests by deleting two that made no sense

c6ae2c6

apatlpo closed this Mar 3, 2020

apatlpo reopened this Mar 3, 2020

rabernat reviewed Mar 6, 2020

View reviewed changes

Aurélien Ponte added 2 commits March 6, 2020 16:26

update tests and doc

ef50559

delete 2.7 in travis.yml

f186a25

try to improve code cov

6fd5e8b

get deprecated methods out of coverage tests

5ed19a6

rabernat merged commit 0ba161f into xgcm:master Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isospectrum debugging / refactoring #89

isospectrum debugging / refactoring #89

apatlpo commented Feb 8, 2020

rabernat commented Feb 8, 2020 •

edited

Loading

apatlpo commented Feb 8, 2020

codecov-io commented Feb 9, 2020 •

edited by codecov bot

Loading

apatlpo commented Feb 9, 2020

apatlpo commented Feb 9, 2020 •

edited

Loading

rabernat commented Feb 15, 2020

apatlpo commented Feb 15, 2020

apatlpo commented Mar 2, 2020 •

edited

Loading

apatlpo commented Mar 3, 2020

roxyboy commented Mar 3, 2020 •

edited

Loading

roxyboy commented Mar 6, 2020 •

edited

Loading

rabernat commented Mar 6, 2020

rabernat left a comment

rabernat Mar 6, 2020

apatlpo Mar 6, 2020

rabernat Mar 6, 2020

rabernat Mar 6, 2020

apatlpo commented Mar 6, 2020

rabernat commented Mar 7, 2020

apatlpo commented Mar 7, 2020

apatlpo commented Mar 10, 2020

rabernat commented Mar 10, 2020

apatlpo commented Mar 10, 2020

rabernat commented Mar 11, 2020

isospectrum debugging / refactoring #89

isospectrum debugging / refactoring #89

Conversation

apatlpo commented Feb 8, 2020

rabernat commented Feb 8, 2020 • edited Loading

apatlpo commented Feb 8, 2020

codecov-io commented Feb 9, 2020 • edited by codecov bot Loading

Codecov Report

apatlpo commented Feb 9, 2020

apatlpo commented Feb 9, 2020 • edited Loading

rabernat commented Feb 15, 2020

apatlpo commented Feb 15, 2020

apatlpo commented Mar 2, 2020 • edited Loading

apatlpo commented Mar 3, 2020

roxyboy commented Mar 3, 2020 • edited Loading

roxyboy commented Mar 6, 2020 • edited Loading

rabernat commented Mar 6, 2020

rabernat left a comment

Choose a reason for hiding this comment

rabernat Mar 6, 2020

Choose a reason for hiding this comment

apatlpo Mar 6, 2020

Choose a reason for hiding this comment

rabernat Mar 6, 2020

Choose a reason for hiding this comment

rabernat Mar 6, 2020

Choose a reason for hiding this comment

apatlpo commented Mar 6, 2020

rabernat commented Mar 7, 2020

apatlpo commented Mar 7, 2020

apatlpo commented Mar 10, 2020

rabernat commented Mar 10, 2020

apatlpo commented Mar 10, 2020

rabernat commented Mar 11, 2020

rabernat commented Feb 8, 2020 •

edited

Loading

codecov-io commented Feb 9, 2020 •

edited by codecov bot

Loading

apatlpo commented Feb 9, 2020 •

edited

Loading

apatlpo commented Mar 2, 2020 •

edited

Loading

roxyboy commented Mar 3, 2020 •

edited

Loading

roxyboy commented Mar 6, 2020 •

edited

Loading