-
Notifications
You must be signed in to change notification settings - Fork 160
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
Issue3438 d xdehumidifier #3475
base: master
Are you sure you want to change the base?
Conversation
Add the unit test results
Addressed inline comments
Issue3438 d xdehumidifier
@terrancelu92: I had a quick look and everything seems to be in order. Can you please review the changes and let @JayHuLBL know if all the changes are expected? Thanks! |
@karthikeyad-pnnl @terrancelu92
Is it possible to downgrade the IDF file, to 9.6? |
"Temperature sensor" | ||
annotation (Placement(transformation(extent={{60,50},{80,70}}))); | ||
|
||
Modelica.Units.SI.MassFraction XOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to calcuate the XOut
here? I also see that we have the sensor senMasFra
and heaFloSen
. Are they required by the model itself? I see that the values are not used anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JayHuLBL Thanks for catching this! I did another review of the code, and realized both XOut
and senMasFra
are not required. However, we are using heaFloSen
to validate the heat transfer of the component within the validation model. It will be useful to identify any errors introduced in the calculation, via CI testing. Please refer to #3788. Thanks!
… issue3438_DXdehumidifier_mergeFix
…er_mergeFix Issue3438 d xdehumidifier merge fix
@@ -0,0 +1,38 @@ | |||
within Buildings.Fluid.Humidifiers.Data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we could have a set of example data in the package. For example, see Buildings.Fluid.HeatPumps.Data.EquationFitReversible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JayHuLBL We actually have an example in the Validation package (Humidifiers.Validation.Data). Should we move it to this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JayHuLBL Is the data from the EnergyPlus reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The parameter values came from the EPlus model that was used for the validation process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add it to the Data
package
…er_mergeFix Issue3438 d xdehumidifier merge fix
…/modelica-buildings into issue3438_DXdehumidifier
This closes #3438.
@karthikeyad-pnnl We are trying to merge the change to master. Would you please take another look to check if the changes are expected?