Issue566weatherfile

[MartinRaetz]

This solves: #566

This should also close this branch: https://github.com/RWTH-EBC/TEASER/tree/issue556_weather

• Branch closed

The dynamic referencing is implemented for the AixLib Export. The other exports should still work the same.
The latest commit implements that the weather file is saved at the "project-level" of the result folder structure.

Checking out the commit 9e75fb2 you will find a version where the weather file is saved at the "building-level" of the result folder structure. This also changes the location where I implemented the changes.

[MartinRaetz]

I think, the failing CI-Test could be due to the reason that this PR actually changes the output of the aixlib export?

=========================== short test summary info ============================
FAILED tests/test_data.py::Test_teaser::test_export_aixlib - FileNotFoundErro...
=========== 1 failed, 346 passed, 128 warnings in 321.30s (0:05:21) ============
ERROR: Job failed: exit code 1

[MichaMans]

@MartinRaetz in my opinion, if you changed everything that the origin state should also work, the tests should not fail. Maybe there is some naming issues?

[MartinRaetz]

Great thanks to @MichaMans for supporting me in fixing the failing CI-Test.
The reason was a Modelica-relative link to an outdated weather file stated in the unitTest.json.
Teaser only supports full-path to locally existing weather files.

@PRemmen: @MichaMans volunteered to review. But, as these changes are to the base of Teaser I thought I would invite you as well. Maybe you can think of a scenario where they fail.

[MartinRaetz]

@marcusfuchs please also see this PR and evaluate if it suits your use-cases. Leave a comment to your stale branch mentioned above, or, if appropriate, delete it.

[coveralls]

Coverage increased (+0.02%) to 90.213% when pulling 1be51b5 on issue566weatherfile into b08a692 on development.

[marcusfuchs]

@MartinRaetz Thanks for addressing this.

Regarding my old branch, I don't think that I have any access to delete this anymore, but feel free to delete it.

Regarding the code, I think this would address our main pain point. If I may make a few suggestions:

• Instead of _help_weather_data I'd suggest a name that more clearly says what the method does, e.g. _copy_weather_file
• In my experience "source" and "destination" are more common wordings than "origin" and "desired"
• If I understand the code correctly, you output the weather file to the top level of the output Modelica package. As both AixLib and the IBPSA library place weather files into <Library>/Resources/weatherdata I think it may be cleaner to also follow this convention with TEASER's output.

[MartinRaetz]

@marcusfuchs thanks for your comment.

I will implement your naming suggestions. I chose the naming _help.... to follow the naming of the other functions in this script. Nevertheless, "copy" is more accurate.

About your third point, the weather file is saved here (Mannheim file):

I can save it in a subfolder "Ressources/Weatherdata" but I do not think it is necessary, since it is not done with the other output files neither. My current approach follows the structure of Teaser and it allows easier checking of the weather file. Let me know, if I got you wrong.

[marcusfuchs]

@MartinRaetz Thanks for picking up my suggestions. Regarding the destination of the weather file it's just my personal opinion that for me things would be cleaner and more user friendly if TEASER shared the same conventions as AixLib and IBPSA. But I can of course also live with your solution of placing the file on the top-level of the output.

[MartinRaetz]

@marcusfuchs

As every user uses its own AixLib and Teaser has no direct connection to the folder system of the AixLib we cannot (and in my opinion should not) modify/copy anything to the AixLib or IBPSA folder structure.

Though, I am not yet sure if I understood you correctly.

Edit: I think I got your point. You suggest building up the same folder structure as known from the AixLib folder structure. I do see the advantage. But I also see that the amount of files is a lot smaller in the Teaser result folder and a folder structure like in the AixLib would be overhead, in my opinion. The current implementation is rather in line with the current Teaser-way of exporting the results.

This discussion affects not only the weather file but all the files "delivered" with the Teaser result folder. I suggest discussing this in a separate issue.

[marcusfuchs]

@MartinRaetz Yes, sorry for the confusion. I'll try to clarify:

In your implementation, you add the weather file on the top-level of the export:

.
├── InstituteBuilding
│   ├── AHU_InstituteBuilding.txt
│   ├── InstituteBuilding_DataBase
│   │   ├── ...
│   │   ├── package.mo
│   │   └── package.order
│   ├── InstituteBuilding.mo
│   ├── InstituteBuilding_Models
│   │   ├── ...
│   │   ├── package.mo
│   │   └── package.order
│   ├── InternalGains_InstituteBuilding.txt
│   ├── package.mo
│   ├── package.order
│   ├── TsetCool_InstituteBuilding.txt
│   └── TsetHeat_InstituteBuilding.txt
├── package.mo
├── package.order
└── WEATHERFILE.mos

I suggest to instead simply move this to Resources/weatherdata/WEATHERFILE.mos:

.
├── InstituteBuilding
│   ├── AHU_InstituteBuilding.txt
│   ├── InstituteBuilding_DataBase
│   │   ├── ...
│   │   ├── package.mo
│   │   └── package.order
│   ├── InstituteBuilding.mo
│   ├── InstituteBuilding_Models
│   │   ├── ...
│   │   ├── package.mo
│   │   └── package.order
│   ├── InternalGains_InstituteBuilding.txt
│   ├── package.mo
│   ├── package.order
│   ├── TsetCool_InstituteBuilding.txt
│   └── TsetHeat_InstituteBuilding.txt
├── package.mo
├── package.order
└── Resources
    └── weatherdata
        └── WEATHERFILE.mos

Not a large change, but IMO a little step towards common conventions.

[MartinRaetz]

@marcusfuchs thank you for clarifying.

In that way we would also need to capsulate all files like TsetCool_InstituteBuilding.txt into a subfolder /ressources/<NameOfTheExperiment>/TsetCool_InstituteBuilding.txt

Like I mentioned above I would prefer having all data visible at a glance, like implemented in the current development and the current commit of this branch. But this might be only my opinion.

Anyhow, this should be discussed in another thread as it affects more than just the weather file.

[MartinRaetz]

@MichaMans thank you for reviewing and helping to improve this PR.

Two things are now added:

• Weather file fix (Problems with Ashrea Weatherfile AixLib#1060)

• Weather file export for the IBPSA export
  (I have tested all verification examples in Teaser, and additionaly simulated all possible results in Modelica. Those of VDI do not produce a model export.)

[MichaMans]

@MartinRaetz Seems to work and looks like a nice addition. I generally agree with @marcusfuchs that a more structured approach might help in the future. Nevertheless I totally agree with you @MartinRaetz that this should be done with a new issue and implementation.

Please merge by squashing the commits 😃