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

Issue1707 updates iso13790 #1882

Open
wants to merge 77 commits into
base: master
Choose a base branch
from
Open

Conversation

amaccarini
Copy link
Contributor

This is to update the validation of the ISO13790 thermal zone model according to the latest BESTEST documentation. Several BESTEST case studies have been added, along with a Python script that automatically generates plots and tables for the user guide. In addition, a new parameter coeFac been added in order to vary the g-factor as a function of the incident angle of the surface.

@amaccarini
Copy link
Contributor Author

@jelgerjansen, I've just finished revising the code. Please let me know if there is anything else that needs improvement.
(Note that there is still an error in the CI, but it seems to be related to the branch not being up to date with the master. The error refers to the file IBPSA_Fluid_Movers_BaseClasses_Validation_EulerCurve.txt).

@jelgerjansen
Copy link
Contributor

@amaccarini I added some other comments to the revision above. Once you've addressed these, I'll resolve those issues as well.

@amaccarini
Copy link
Contributor Author

@jelgerjansen Thanks for the additional comments! I've just revised the code accordingly. Please let me know if there is anything else. If not, I will start working on the conflict that should be resolved before merging.

@amaccarini
Copy link
Contributor Author

@EttoreZ, thanks a lot for your help in resolving the conflict!

@mwetter, there are two errors in the CI tests, but I can't figure out the cause. The errors seem to relate to models that I haven't made any changes to. Could you take a look?

@mwetter
Copy link
Contributor

mwetter commented Sep 17, 2024

@amaccarini : Yes, this way we can later remove it when CDL is ported to IBPSA without incurring a backward incompatible change.

@amaccarini
Copy link
Contributor Author

@mwetter, @jelgerjansen: I removed the block from IBPSA.Utilities.Math and added its source code into the relevant BESTEST cases.

@jelgerjansen
Copy link
Contributor

jelgerjansen commented Sep 19, 2024

@amaccarini thanks! I noticed you also copied the full documentation section and revision history. Maybe you can replace all of this by referring to the corresponding model in the Modelica Buildings library in the documentation section?
@mwetter what do you think?
Nevertheless, I'll already approve all changes.

@mwetter
Copy link
Contributor

mwetter commented Oct 8, 2024

Michael to have a last look and then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants