xgcm / aerobulk-python

A python wrapper for aerobulk (https://github.com/brodeau/aerobulk)
GNU General Public License v3.0
14 stars 4 forks source link

Fix 'slp' typo in skin_np() #53

Closed paigem closed 1 year ago

paigem commented 1 year ago

The order of the input variables for skin_np() is wrong, with the optional slp variable before the required arguments rad_sw and rad_lw.

This PR updates the location of the slp variable in skin_np() so that slp comes before all optional input variables, and also matches the input order of skin(). For clarity, the input ordering for both skin() and skin_np() is:

def skin(
    sst,
    t_zt,
    hum_zt,
    u_zu,
    v_zu,
    rad_sw,
    rad_lw,
    slp=101000.0,
    algo="coare3p0",
    zt=2,
    zu=10,
    niter=6,
    input_range_check=True,
)

This typo was found today with the help of @shanicetbailey after I kept crashing my Python kernel trying to run skin.

jbusecke commented 1 year ago

I am not sure I understand the issue in the first place. The _np functions only have positional arguments, there are no optional inputs on the numpy level.

I want to first understand the error you are seeing better. Can you run the computation which crashed your kernel in a python script in the terminal, so we see some error output?

A word of caution on the ordering (which IMO should stay as is for at least the _np functions): This bit here requires the slp to be in front of the rad_* terms, or it would not work on both skin_np and noskin_np inputs (in the latter case this loop simply ends after the slp).

This could still mean that we got something wrong, but then the ordering needs to be switched somewhere here instead.

Suggested next steps:

  1. Figure out what is actually causing the error
  2. Double check our test inputs/outputs. If we convice ourselves that we are not making a mistake in our testing, we should not merge anything that does not pass (which this PR currently doesnt).

Happy to have a short chat offline later, if needed.

paigem commented 1 year ago

Thanks for the comment @jbusecke - I agree that I was too quick to make this PR. 😬

I have figured out the problem: I had input_range_check=False on yesterday and so, when I had slp in the wrong order, it crashed the kernel. By turning on input_range_check=True I am getting errors that make it clear that slp should be passed in before the radiation terms and then it runs no problem.

Sorry for this unnecessary PR! I'll close now.

jbusecke commented 1 year ago

But just to clarify, the docstrings are correct? Or do we need to change something in there?