Skip to content
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

Fix gufunc signature #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rabernat
Copy link
Collaborator

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.

def test_calc_cape_pressure(dataset_ERA5pressurelevel, sourcein, pinc_used, ml_depthin, use_dask,vertical_levin):

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
Copy link
Collaborator Author

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
Copy link

codecov bot commented Dec 10, 2021

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%) ⬆️

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant