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

Issue1389 detailed house #1404

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

Conversation

LoneMeertens
Copy link

@LoneMeertens LoneMeertens commented Dec 23, 2024

Fixes #1389.
More detailed documentation and result visualization can be added if deemed valuable. This can be combined with open-ideas/__CrashCourse__#25. and #1374

@lucasverleyen
Copy link
Member

lucasverleyen commented Dec 24, 2024

@LoneMeertens, thank you for addressing the issue! I really like the new structure, a great improvement. :-) Also, adding more documentation/explanation provides significant added value.

  • I suggest including Refactoring exercise 3 __CrashCourse__#25 and Update Tutorial IDEAS #1374 as well in this PR, which seems more convenient than having too many issues and PRs related to the same models.
  • Regarding more detailed documentation and result visualisation, I suggest maintaining consistency with the SimpleHouse tutorial from IBPSA and following a similar approach. This tutorial has been thoroughly reviewed recently and is consequently in good shape.
  • Additionally, we can add DetailedHouse10 as a final Example at the same level as SimpleHouse.

@LoneMeertens
Copy link
Author

LoneMeertens commented Dec 24, 2024

@lucasverleyen Thank you for the detailed feedback and suggestions! 😊 I'm glad you like the new structure and the additional documentation.

I’ll take this up and include open-ideas/CrashCourse#25 and Update Tutorial IDEAS #1374 in this PR as suggested—it does make sense to consolidate related issues and PRs.

Regarding the documentation and result visualization, I'll align it with the SimpleHouse tutorial from IBPSA to ensure consistency and maintain the high standard of quality. I'll also add DetailedHouse10 (as DetailedHouse) as a final example, positioned at the same level as SimpleHouse.

Thanks again for the valuable input!

@lucasverleyen
Copy link
Member

@LoneMeertens perfect! Thanks a lot! In the mean time, I have looked at the unit tests, but currently, I am not able to solve the issues. DetailedHouse5 does work, but the other examples do not and I don't know why... to be continued in the new year ;-)

@lucasverleyen
Copy link
Member

lucasverleyen commented Jan 2, 2025

@jelgerjansen good catch (__Dymola_Commands)! I suggest adding this to the documentation of the testing framework: https://github.com/open-ideas/IDEAS/blob/master/IDEAS/Resources/Scripts/tests/README.md under Developing new tests. Something like: "Make sure that the file name and path in the model annotation under __Dymola_Comands correspond to the name and path to the .mos file in Resources/Scripts/Dymola/... " What do you think?

Copy link
Contributor

@jelgerjansen jelgerjansen left a comment

Choose a reason for hiding this comment

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

@LoneMeertens thank you for addressing this issue. Most comments are related to model documentation and syntax.

The models previously didn't run due to the reference to an unexisting mos file in __Dymola_Commands(). This does not generates warnings when translating the model in Dymola, but it does for the unit tests. I solved this issue via commits 8cca500 and e8f49c5

Once you've also addressed the other related issues (e.g. #1374), I'll do another review.

IDEAS/Examples/Tutorial/DetailedHouse/DetailedHouse1.mo Outdated Show resolved Hide resolved
This first example file instantiates a simple building model that consists of one zone, four walls,
a window, a floor and a ceiling. The zone dimensions are 8 m (with walls oriented
north and south) by 4 m, and the window measures 3 m by 1.4 m. Use the default
zone height of 2.8 m. Apply double glazing and a heavy wall, which provide high thermal mass.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very specific. Should we refer to the exact types?

IDEAS/Examples/Tutorial/DetailedHouse/DetailedHouse2.mo Outdated Show resolved Hide resolved
IDEAS/Examples/Tutorial/DetailedHouse/DetailedHouse4.mo Outdated Show resolved Hide resolved
IDEAS/Examples/Tutorial/DetailedHouse/DetailedHouse7.mo Outdated Show resolved Hide resolved
IDEAS/Examples/Tutorial/DetailedHouse/DetailedHouse9.mo Outdated Show resolved Hide resolved
IDEAS/Examples/Tutorial/DetailedHouse/package.mo Outdated Show resolved Hide resolved
IDEAS/Examples/Tutorial/DetailedHouse/package.mo Outdated Show resolved Hide resolved
@jelgerjansen
Copy link
Contributor

@jelgerjansen good catch (__Dymola_Commands)! I suggest adding this to the documentation of the testing framework: https://github.com/open-ideas/IDEAS/blob/master/IDEAS/Resources/Scripts/tests/README.md under Developing new tests. Something like: "Make sure that the file name and path in the model annotation under __Dymola_Comands correspond to the name and path to the .mos file in Resources/Scripts/Dymola/... " What do you think?

@lucasverleyen the __Dymola_Commands is not used when running the unit tests, but links the mos file to the model such that the experiment in the mos file is executed when running the "Simulate and plot" command in Dymola. Note that you don't need a __Dymola_Commands to run the unit tests (which is for example why DetailedHouse5 could be run without problems), but is easier if you want to run unit tests mos files of certain models in Dymola. Therefore, I would maybe not add it, as it is no hard requirement.

If you add it, the file in __Dymola_Commands should of course be linked to the correct mos file (but this is rather a general requirement rather than a unit test requirement). I think that BuildingsPy performs a more detailed check of the model when translating compared to Dymola, as the latter did not throw any errors.

@LoneMeertens
Copy link
Author

@jelgerjansen @lucasverleyen I have addressed the mentioned comments and added a 'Connection Instructions' and 'Reference Result' section to each sub DetailedHouse model, to align with the SimpleHouse example. Additionally, the related issues (e.g., #1374) have been resolved. Thank you for reviewing this! Please let me know if there are any minor adjustments needed, and I will be happy to make them.

IDEAS.Buildings.Components.Window</a>
</li>
<li>
<a href=\"modelica://IDEAS.Buildings.Components.InternalWall\">
Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember, we decided last year to remove the internal wall representing the floor and ceiling and use a slab on ground for the floor and another outer wall for the ceiling. This was indeed not the case in this tutorial, but I would change the tutorial according to the changes of last year in the crash course exercise

<img alt=\"Zone temperature as function of time.\"
src=\"modelica://IDEAS/Resources/Images/Examples/Tutorial/DetailedHouse/DetailedHouse1.png\" width=\"700\"/>
</p>
</html>", revisions="<html>
Copy link
Member

Choose a reason for hiding this comment

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

You can update the revision history of each model, with a reference to this pull request or the issue

</ul>
<h4>Connection instructions</h4>
<p>
Set the appropriate replaceable models in the zone model.
Copy link
Member

Choose a reason for hiding this comment

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

From the last crash course, I remember that it was not clear that you could simply set this in the zone model's dialogue window. Maybe you can specify this in this sentence by adding "dialogue window"?

@@ -165,7 +163,76 @@ First implementation for the IDEAS crash course.
</ul>
</html>", info="<html>
<p>
Adding CO2-controlled ventilation system.
Adding CO2-controlled ventilation system. The occupancy model
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adding CO2-controlled ventilation system. The occupancy model
Adding CO<sub>2</sub>-controlled ventilation system. The occupancy model

Copy link
Member

Choose a reason for hiding this comment

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

can you add sub- and superscripts for CO2 and the units (m2, m3, ...)

IDEAS.Examples.Tutorial.DetailedHouse.DetailedHouse4</a> is added to one zone and a
fixed occupancy of 1 person to the other zone. The ventilation system
consists of two fans, two supply and two return air VAVs (Variable Air Volume), a heat recovery unit and an
outdoor air source. The control consists of PI controllers with a setpoint of 1000 ppm.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
outdoor air source. The control consists of PI controllers with a setpoint of 1000 ppm.
outdoor air source. The control consists of PI controllers with a setpoint of 1000 <i>ppm</i>.

@lucasverleyen
Copy link
Member

@LoneMeertens thanks a lot for all improvements! I have added a few more comments. I see there are still some comments by Jelger that are open. The easiest is to click on "Resolve conversation" when you have addressed the comment, and then it is clear which comments have been addressed.

@lucasverleyen lucasverleyen added this to the Release 3.x milestone Jan 10, 2025
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.

Create separate package within IDEAS.Examples.Tutorial for Modelica crash course examples
3 participants