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

Issue582 update testcase1 and testcase 3 documentation #706

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

Conversation

Jaap-Neven
Copy link
Contributor

Written basic documentation of the layout of testcase 1 and testcase 3. Included resources to produce a pdf from latex and the most recent pdf version as well.

Did not refactor the names of the basic p-controllers in the example controllers directory and corresponding references yet. Should this be done or unnecessary?

@Jaap-Neven Jaap-Neven changed the title Issue582 Update testcase1 and testcase 3 documentation Issue582 update testcase1 and testcase 3 documentation Nov 15, 2024
@dhblum
Copy link
Collaborator

dhblum commented Dec 4, 2024

This is for #582.

@Jaap-Neven Thanks for the PR! A couple comments and requests:

  1. Instead of latex, pdf, and all the build files, can you provide the documentation of the test cases similarly to the other test cases and as defined in the design spec (see here)? That is, add the documentation as an html annotation in the Modelica model (for example see for the bestest_air case here) and also copy to a /testcases/<test_case_name>/doc/index.html, with any figures in a /testcases/<test_case_name>/doc/images/ directory. I suggest looking at the other test cases for templates (again see for example the bestest_air case here). This will align the docs with how things are done in other test cases, and also avoid needing to build, add, and use pdfs in git.
  2. One of the things mentioned in #582 was the heater should be considered as a heater/cooler. Therefore, can you change the measurement variable PHea_y to PHeaCoo_y? That will require a change in the Modelica model overwrite block, cascade into needing to update unit test results with the new variable, reflected in documentation, and maybe updates to the scripts in /examples.
  3. Add a note of the updates to releasenotes.md under a section that is "Not backwards compatible but does not change benchmark results." Use previous release notes as a template. I'm saying this is not backwards compatible because of the change in measurement point name.

@dhblum dhblum self-requested a review December 4, 2024 15:43
Copy link
Collaborator

@dhblum dhblum left a comment

Choose a reason for hiding this comment

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

See previous comment for review requests.

@Jaap-Neven
Copy link
Contributor Author

Hi @dhblum, thanks for the feedback! I'll implement the requested changes at my earliest convenience.

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.

2 participants