chmarti1 / PYroMat

PYroMat thermodynamic properties in Python
http://pyromat.org
Other
71 stars 13 forks source link

Modifications in response to issue #52 to go back to original call si… #53

Closed jranalli closed 1 year ago

jranalli commented 2 years ago

closes #52. Modifications were made as simply as possible and used a similar method to argparse.

Check for the presence of the variable of interest in vargs, and if it's there, move it into kwargs. Then call the followup function without referencing vargs. Only applies when p is the first positional argument, because argparse already assumes T is the first positional argument.

        if len(varg) > 0:
            if 'p' in kwarg:
                raise pm.utility.PMParamError('p was specified both positionally and with a keyword.')
            kwarg['p'] = varg[0]
        if len(varg) > 1:
            raise pm.utility.PMParamError('There are only two positional arguments: h, p.')

Some test code to ensure reliability. Any other calls already rely on kwargs and should be fine.

def test_bug_git52(self):
        # ig, ig2, igmix, mp1
        subs = [pm.get("ig.BH3O3"), pm.get("ig.O2"), pm.get('ig.air'), pm.get('mp.H2O')]

        for sub in subs:
            p = 1
            T = 500
            h = sub.h(T=T, p=p)
            s = sub.s(T=T, p=p)
            assert sub.T_s(s, p) == approx(sub.T_s(s, p=p))
            assert sub.T_s(s, p) == approx(sub.T_s(s=s, p=p))
            assert sub.T_h(h, p) == approx(sub.T_h(h, p=p))
            assert sub.T_h(h, p) == approx(sub.T_h(h=h, p=p))
            if hasattr(sub, "p_s"):
                assert sub.p_s(s, T) == approx(sub.p_s(s, T=T))
                assert sub.p_s(s, T) == approx(sub.p_s(s=s, T=T))
            if hasattr(sub, "d_s"):
                assert sub.d_s(s, T) == approx(sub.d_s(s, T=T))
                assert sub.d_s(s, T) == approx(sub.d_s(s=s, T=T))
jranalli commented 2 years ago

As I'm looking at it I think we want to retain the kwarg part. If the goal is to match the old signatures for T_s, etc. I think you need to retain the quality=True flag for mp1, and the ability to use density.

So I think for mp1 it would be like:

        if p is not None:
            T,_,_,x,_ = self._argparse(s=s, p=p)
        else:
            T,_,_,x,_ = self._argparse(s=s, **kwarg)
        if quality:
            return T,x
        return T

And for ig:

        if p is not None:
            return self.T(s=s, p=p)
        return self.T(s=s, **kwarg)

Does that look right to you? If so, I'm happy to make the edits.

chmarti1 commented 2 years ago

True. I think I'd like to weave this into my own working branch and play with it a bit before we fold it in.

jranalli commented 2 years ago

Ok, just committed the changes that would follow the methodology proposed there if you want to use these as a simpler starting point. Or you can just back off by a commit or start from wherever you like!

chmarti1 commented 1 year ago

Whew. It really took me a while to get back to this. I agree with your edits - this is the right way to do it. I'm approving this pull request.