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

air.T_s claims temperature out-of-bounds when using reverse values from air.s #52

Closed
danieljuschus opened this issue Sep 13, 2022 · 4 comments · Fixed by #53
Closed

Comments

@danieljuschus
Copy link

import pyromat as pm
air = pm.get("ig.air")
s = air.s(250,1)
air.T_s(s,1)

should result in 250, but raises the following error: PMParamError: All of the specified states were out-of-bounds. Legal temperatures for ig.air are between 200.0 and 6000.0 Kelvin.

@jranalli
Copy link
Collaborator

So the problem there has to do with the way PyroMat interprets arguments that aren't explicitly specified. As of ver 2.2, the first positional argument is always assumed to be T and the second p. Since T_s is a legacy function, it does retain the first argument as s, but then the next one is immediately interpreted as T. So what's really being done in your function call is: air.T_s(s=s, T=1)

I'll let @chmarti1 speak up as to how he wants to deal with the function headers on stuff like this, but you could fix this in your code right away by specifying explicitly air.T_s(s, p=1). I would suggest that it's almost always better to be explicit with the args to avoid any confusion.

Of note as well is that since 2.2, the calls like T_s aren't necessary anymore, because the underlying argument parser handles all possible arg pairs. So calling T_s is the same as calling air.T(s=s, p=1), with the latter being more straightforward and preferred at this point. It does mean updating code, but that ought to be more stable.

@chmarti1
Copy link
Owner

chmarti1 commented Sep 13, 2022

@jranalli is right - this is a small departure from the legacy T_s() behavior. Technically, it breaks reverse compatibility in a way that is undocumented, which makes it a bug. @jranalli's fix absolutely works. Meanwhile, I'd like to push an update that reverts to the original call signatures of the T_s() and T_h() inverse methods.

In a related note, I just noticed a typo in the documentation that calls these function "Depreciated" instead of "Deprecated" (oops). The point is the same. We'll keep the T_? inverse routines for all version 2 code, but future applications should avoid them.

@chmarti1 chmarti1 added the bug label Sep 13, 2022
@chmarti1
Copy link
Owner

I've looked at your code. It looks like line 117 is the only one that needs a fix to quickly avoid the issue. I recommend just changing it to read

t3_is = float(air.T(s=s1, p=p3))

That will work with version 2.2.1.

If you want, you can also abandon def p_s_theo() on line 26. PYroMat now supports p(s,T) call signatures.

Thanks for calling this to our attention!

@danieljuschus
Copy link
Author

Thanks! That worked.

jranalli added a commit that referenced this issue Sep 16, 2022
jranalli added a commit that referenced this issue Sep 16, 2022
…gnatures for deprecated T_s, etc. function calls.
jranalli added a commit that referenced this issue Sep 16, 2022
chmarti1 added a commit that referenced this issue Oct 10, 2022
Modifications in response to issue #52 to go back to original call si…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants