add TVC model #1516
add TVC model #1516
Conversation
|
The failures are due to #1514 and are most likely unrelated to the changes in this PR. |
|
@ElmiraShamlou, just needs to add some documentation. |
| # Empirical correlation constraints | ||
| @self.Constraint(doc="Pressure Correction Factor (El-Dessouky, 1997)") | ||
| def eq_PCF(b): | ||
| A_PCF = 3e-7 / pyunits.kPa**2 |
There was a problem hiding this comment.
Is there a reason not to create/declare the parameters for PCF and TCF as model variables as is the usual practice?
There was a problem hiding this comment.
Or at least as model parameters? Might there ever be a case where these coefficients would change due to parameter estimation using a new dataset?
There was a problem hiding this comment.
Defining them as Param or Var is unnecessary because PCF and TCF are fully derived from other variables through fixed empirical relationships
There was a problem hiding this comment.
I have some quick, minor comments and will aim to take another pass after having checked the reference paper(s). However, the main missing piece now is documentation for the unit model. I'd recommend pushing what you have and potentially tagging someone to support on polishing that off.
| # Empirical correlation constraints | ||
| @self.Constraint(doc="Pressure Correction Factor (El-Dessouky, 1997)") | ||
| def eq_PCF(b): | ||
| A_PCF = 3e-7 / pyunits.kPa**2 |
There was a problem hiding this comment.
Or at least as model parameters? Might there ever be a case where these coefficients would change due to parameter estimation using a new dataset?
Co-authored-by: Adam Atia <[email protected]>
Co-authored-by: Adam Atia <[email protected]>
Co-authored-by: Adam Atia <[email protected]>
Co-authored-by: Adam Atia <[email protected]>
There was a problem hiding this comment.
LGTM - I agree with @MarcusHolly 's suggestions on the documentation and just think you should make those changes before we merge.
Co-authored-by: Adam Atia <[email protected]>
Co-authored-by: Adam Atia <[email protected]>
|
It seems like in the latest commit (refine doc), you accidentally undid the changes to spelling out TCF & PCF as well as correcting the degrees of freedom typo. Once this is addressed, I'll approve. |
ElmiraShamlou
requested review from
adam-a-a
and removed request for
adam-a-a
Fixes/Resolves:
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)
Summary/Motivation:
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: