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 'slp' typo in skin_np() #53

Closed
wants to merge 1 commit into from
Closed

Fix 'slp' typo in skin_np() #53

wants to merge 1 commit into from

Conversation

paigem
Copy link
Collaborator

@paigem paigem commented Oct 24, 2022

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

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

paigem commented Oct 25, 2022

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.

@paigem paigem closed this Oct 25, 2022
@jbusecke
Copy link
Contributor

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

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.

2 participants