xgcm / xcape

Fast convective parameters for numpy, dask, and xarray
Other
30 stars 11 forks source link

Fix gufunc signature #51

Open rabernat opened 2 years ago

rabernat commented 2 years ago

This PR should fix the failing test by fixing what I think was a bug in input handling.

It is extremely unlikely that this bug caused any calculations to be wrong! It's more about fixing an error message that will be incorrectly generated when using pressure levels.

What was the problem?

Essentially, the problem seems to be that we used to have two different gufunc signatures depending on whether vertical_lev was sigma or pressure, indicating that the core functions (e.g. _calc_cape_numpy) should have either 6 (sigma) or 7 (pressure) arguments. However, in the tests calling this code, e.g.

https://github.com/xgcm/xcape/blob/1cd1418423b131ddd74ce11f3d1091ecabc83a29/test/test_core.py#L125

we only ever have 6 arguments: p, t, td, ps, ts, tds.

This PR should make all the tests pass by simply removing the special case for pressure.

Why was it working before (in python 3.6)? My theory is that the older version of dask in that env simply didn't check whether the inputs passed to apply_gufunc were consistent with the signature.

How I fixed it

I just made the gufunc signature the same for both sigma and pressure level.

How you can help

@chiaral & @xebadir: can you please share any code examples you have of using xcape with pressure-level data? I am trying to figure out why we had this special case for pressure level inputs. Can you think of any reason why we might have thought that pressure level data would need one more input argument than sigma level data?

rabernat commented 2 years ago

p.s. Unfortunately my editor made lots of whitespace changes, which resulted in a large diff. I hate this, and it makes this PR hard to review.

We need to set up pre-commit and black to keep the code style consistent.

codecov[bot] commented 2 years ago

Codecov Report

Merging #51 (02b9e85) into master (1cd1418) will decrease coverage by 0.11%. The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   66.78%   66.66%   -0.12%     
==========================================
  Files           9        9              
  Lines         283      276       -7     
  Branches       74       70       -4     
==========================================
- Hits          189      184       -5     
  Misses         74       74              
+ Partials       20       18       -2     
Impacted Files Coverage Δ
src/xcape/core.py 83.89% <73.33%> (+0.55%) :arrow_up:

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 1cd1418...02b9e85. Read the comment docs.