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

Use of triple point of water in GetSatVapPres #36

Merged
merged 6 commits into from
Aug 8, 2019
Merged

Use of triple point of water in GetSatVapPres #36

merged 6 commits into from
Aug 8, 2019

Conversation

dmey
Copy link
Contributor

@dmey dmey commented Jul 30, 2019

This PR introduces the use of triple point of water in GetSatVapPres to make the functions more physically correct and simplifies considerably the NR convergence procedure in GetTDewPointFromVapPres. This issue arises from the discontinuity at the freezing point of water.

didierthevenard and others added 5 commits July 26, 2019 13:01
This change is more physically correct and simplifies considerably the NR convergence procedure in GetTDewPointFromVapPres.
This change is more physically correct and simplifies considerably the NR convergence procedure in GetTDewPointFromVapPres.
@dmey dmey added this to the 2.2.0 milestone Jul 30, 2019
@dmey
Copy link
Contributor Author

dmey commented Jul 30, 2019

@didierthevenard should the other couple of instances of freezing point of water also be replaced?
E.g.

if TWetBulb >= 32:

If not, to make the code cleared we should probably make this into a parameter and call it something like FREEZING_POINT_WATER_{SI, IP}

@@ -949,6 +940,12 @@ def GetSatVapPres(TDryBulb: float) -> float:

Reference:
ASHRAE Handbook - Fundamentals (2017) ch. 1 eqn 5 & 6
Important note: the ASHRAE formulae are defined above and below the freezing point but have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, this comment should be copied to all other languages.

Copy link
Contributor

@didierthevenard didierthevenard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change requested - propagate to other languages the comment made in Python about the justification for using the triple point of water. Other than that, no issue found.

@didierthevenard
Copy link
Contributor

Regarding comment #36 (comment): I have checked, and please tell me if I am wrong, but I can find only one place where the freezing point of water is used, that is, in GetHumRatioFromTWetBulb(). I have checked and the formulae in that function do present a discontinuity at the freezing point. However changing the threshold to the triple point does not resolve the discontinuity so I am not sure if this is related at all to the issue we had in GetSatVapPres() - likely not. So no change needed. The discontinuity does not seem to cause any issues anyway. As for changing the values 32 and 0 to FREEZING_POINT_WATER I think it would be a good idea. For consistency I suggest that the constant could be declared at the same time as TRIPLE_POINT_WATER, despite the fact that it is used only in one function and could be declared there locally instead.

@dmey
Copy link
Contributor Author

dmey commented Aug 5, 2019

Regarding comment #36 (comment): I have checked, and please tell me if I am wrong, but I can find only one place where the freezing point of water is used, that is, in GetHumRatioFromTWetBulb(). I have checked and the formulae in that function do present a discontinuity at the freezing point. However changing the threshold to the triple point does not resolve the discontinuity so I am not sure if this is related at all to the issue we had in GetSatVapPres() - likely not. So no change needed. The discontinuity does not seem to cause any issues anyway. As for changing the values 32 and 0 to FREEZING_POINT_WATER I think it would be a good idea. For consistency I suggest that the constant could be declared at the same time as TRIPLE_POINT_WATER, despite the fact that it is used only in one function and could be declared there locally instead.

That's right -- agreed. And sure, have now added FREEZING_POINT_WATER_{IP, SI}.

Propagated note about using the triple point of water to the other languages and I retested VBA locally. I have re-requested your review. After this we can prepare for the new release. Let me know if you want to consider this a bugfix or not as this to me would be a patch version 2.1.1 -- i.e. point 3 from the list below (https://semver.org/):

1. MAJOR version when you make incompatible API changes,
2. MINOR version when you add functionality in a backwards-compatible manner, and
3. PATCH version when you make backwards-compatible bug fixes.

Copy link
Contributor

@didierthevenard didierthevenard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found, thanks for your work on this!
As for the version, yes this can be a patch.

@dmey dmey merged commit 1c80701 into master Aug 8, 2019
@dmey dmey deleted the TriplePoint branch August 8, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants