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.

Sign up for GitHub

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

Jump to bottom

Regression tests: Option to use reference results from custom path? #393

Open
marcusfuchs opened this issue Nov 18, 2020 · 5 comments · May be fixed by #395
Open

Regression tests: Option to use reference results from custom path? #393

marcusfuchs opened this issue Nov 18, 2020 · 5 comments · May be fixed by #395

Comments

@marcusfuchs
Copy link

From my understanding of the code in buildingspy/development/regressiontest.py, the reference results for the regression testing are always read from the following directory:

        # Check if the directory
        # "self._libHome\\Resources\\ReferenceResults\\Dymola" exists, if not
        # create it.
        refDir = os.path.join(self._libHome, 'Resources', 'ReferenceResults', 'Dymola')
        if not os.path.exists(refDir):
            os.makedirs(refDir)

We have a use case where we would like to use BuildingsPy to run tests on an auto-generated Modelica package. For this application, it would be useful if we could specify a base directory for the reference results which is outside of the generated library. This may e.g. be done by adding a new class attribute self.refDir and moving the code snippet mentioned above into the class initialization to execute it when the default refDir=None is passed to the ìnit` method.

As I understand that this proposed change may be a bit too special for all users I first wanted to ask whether there is an interest in me implementing this. As an alternative, we could also copy reference results into the library during the auto-generation process, so we could also work around this issue.
@mwetter
Copy link
Member

mwetter commented Nov 18, 2020

@marcusfuchs : I think such an option would be useful. As requirements, we would like to be able to

  • specify a path for where the get data.
  • easily say to run the regression test with tool A but verify against time series from tool B. In this case, the size of the system of equations should not be compared (as they rely on tool-dependent heuristics).

marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Nov 18, 2020
@marcusfuchs
Move reference directory definition to init
970954b 
For lbl-srg#393
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Nov 18, 2020
@marcusfuchs
Move default dir_ref and folder creation
707869c 
This needs to happen after setting the library root, not in init. For lbl-srg#393
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Nov 18, 2020
@marcusfuchs
Extract setup of reference directory to method
0cc6b95 
For lbl-srg#393
@marcusfuchs
Copy link
Author

@mwetter Great, I can make a suggestion then.

I already saw that what I suggested above will not work correctly. It does not seem possible to set up the default reference directory path during initialization, because the path depends on the library root (self._libHome). This is not set at init, but in setLibraryRoot. So this is also where we have to set up the reference directory if no custom path is given. I'll open a PR as WIP for that.

Regarding your other point about comparing different tools, I am not yet sure how this would be best implemented. It seems to me like currently, the reference result directory is always refDir = os.path.join(self._libHome, 'Resources', 'ReferenceResults', 'Dymola') from which I assume that simulated results can currently only be compared against Dymola results? Unfortunately, it's been a while since I looked into other tools, so my understanding here may be too limited.

@marcusfuchs marcusfuchs mentioned this issue Nov 18, 2020
Open
@mwetter
Copy link
Member

mwetter commented Nov 20, 2020

@marcusfuchs : Thanks for the PR. Let me have a look at it and propose some edits for #393 (comment)

@mwetter
Copy link
Member

mwetter commented Nov 20, 2020

@marcusfuchs : I made a working branch issue393_customrefs.

Can you please have a look at whether the new code meets what you need.

The following will still need to be done (most likely in December):

  • Write regression tests for the new functionality
    • compare with results outside of the library
    • compare results between Dymola and optimica for MyModelicaLibrary/Examples
    • write a tests that ensures failure if both new arguments base_reference_result_directory and base_reference_result_tool are specified.
    • write a test that ensures failure if base_reference_result_directory does not exist

@mwetter mwetter linked a pull request Nov 20, 2020 that will close this issue
Draft
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Dec 1, 2020
@marcusfuchs
Extract tool name processing to method
3af75a0 
For lbl-srg#393
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Dec 1, 2020
@marcusfuchs
Refactor conditional
575bf85 
For lbl-srg#393
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Dec 1, 2020
@marcusfuchs
Add test for error too many settings
c75ab45 
For lbl-srg#393
marcusfuchs added a commit to marcusfuchs/BuildingsPy that referenced this issue Dec 1, 2020
@marcusfuchs
Add test for non-existing directory
907ba79 
For lbl-srg#393
@marcusfuchs
Copy link
Author

marcusfuchs commented Dec 1, 2020 •
edited
Loading

@mwetter Sorry that it took a little longer on my side, but now I had a look at your code. I think your additions still cover my use case nicely, so I think this is even better now.

Regarding the logic for the different options, I made some suggestions for a refactoring of your code. The functionality should not have been changed, but I find the conditional statements a little easier to understand in my version. In addition, I added 2 of the tests you suggested above, so what's left is the following

Write regression tests for the new functionality:

  • compare with results outside of the library
  • compare results between Dymola and optimica for MyModelicaLibrary/Examples
  • write a tests that ensures failure if both new arguments base_reference_result_directory and base_reference_result_tool are specified.
  • write a test that ensures failure if base_reference_result_directory does not exist

If you agree with my suggestions the current code would work for me. If you prefer your version, I could also revert my refactorings and just include the new tests.

My new code is already updated in #394

@marcusfuchs marcusfuchs mentioned this issue Dec 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add option to compare results across tools lbl-srg/BuildingsPy
2 participants
@mwetter @marcusfuchs