search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: AixCaliBuHA: Automated calibration of building and HVAC systems #3861

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@FWuellhorst<!--end-author-handle-- (Fabian Wüllhorst) Repository: https://github.com/RWTH-EBC/AixCaliBuHA Branch with paper.md (empty if default branch): Version: v_0.3.0 Editor: !--editor-->@fraukewiese<!--end-editor-- Reviewers: @samanmostafavi, @shamsiharis Archive: 10.5281/zenodo.6475439

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/ef57939010ca99df11bf4c2056dd7caf"><img src="https://joss.theoj.org/papers/ef57939010ca99df11bf4c2056dd7caf/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/ef57939010ca99df11bf4c2056dd7caf/status.svg)](https://joss.theoj.org/papers/ef57939010ca99df11bf4c2056dd7caf)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@@shamsiharis & @samanmostafavi, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @fraukewiese know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @shamsiharis

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @samanmostafavi

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ralphevins, @samanmostafavi it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago

Wordcount for paper.md is 908

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.14 s (349.6 files/s, 170319.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
SVG                              5              0            135          17082
Python                          26            579           1552           2648
Markdown                         5             73              0            248
Jupyter Notebook                 1              0            566            153
TeX                              1             11              0            123
reStructuredText                 7             34             66             43
YAML                             2              3              4             42
make                             1              4              6             10
-------------------------------------------------------------------------------
SUM:                            48            704           2329          20349
-------------------------------------------------------------------------------

Statistical information for the repository '2dce1dc2e9f342b8f938c3eb' was
gathered on 2021/10/27.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Fabian                           1             5             10            0.05
Fabian Wüllhorst                10          6304           5805           37.37
Leonard Schulte                  1             4              4            0.02
Lovis Kauderer                   6           131             52            0.56
MBaranskiEBC                     3           913              0            2.82
MichaMans                        1            12              6            0.06
Philipp Mehrfeld                 3             9              2            0.03
Sebastian                       22           904            692            4.93
Sebastian Borges                 1            39             38            0.24
Sebastian.Borges                 1            10              1            0.03
fabian.wuellhorst              163          8663           6126           45.65
philipp.mehrfeld                36          1291           1067            7.28
zhiyupan                         1           309              3            0.96

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
FWuellhorst                1227          100.0         19.0               14.75
Lovis Kauderer                4            3.1         10.1                0.00
MichaMans                     1            8.3          0.9                0.00
Philipp Mehrfeld              5           55.6         30.2               40.00
Sebastian.Borges            131         1310.0         11.1               25.19
fabian.wuellhorst          3408           39.3          6.7               16.70
zhiyupan                      3            1.0         28.1               66.67
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.00097 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1109/ACCESS.2020.2990567 is OK
- 10.1016/j.rser.2014.05.007 is OK
- 10.26868/25222708.2019.210992 is OK
- 10.3384/ecp21181561 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fraukewiese commented 3 years ago

@ralphevins @samanmostafavi - This is the review thread for the paper. All of our communications will happen here from now on.

Please read the "Reviewer instructions & questions" in the first comment above.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention #3861 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.

fraukewiese commented 3 years ago

@ralphevins @samanmostafavi : If there are any questions regarding the review, do not hesitate to ask.

whedon commented 3 years ago

:wave: @ralphevins, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @samanmostafavi, please update us on how your review is going (this is an automated reminder).

shantropy commented 2 years ago

Hi @fraukewiese, I'll review your software in the next few days, and open up issues in the repository as I go along. Once finished, I'll summarize back here.

ralphevins commented 2 years ago

@fraukewiese I have a question. The software in question is an API to Dymola, a (very expensive) commercial product. As is, it opens up Dymola in the system and returns 'Can't find AixLib library' error. The free version of Dymola cannot support AixLib because the trial version supports limited state-space equations and full models in the library can't be translated or simulated. So unfortunately it looks as if we must reject, as it's not possible to review (or use) the library without a Dymola license?

FWuellhorst commented 2 years ago

@fraukewiese @ralphevins : First of all thanks for taking the time to review our software code.

To explain the issue here: You probably started with the first example, which is just there to help users analyze their system and identify inputs, outputs, and parameters of the model they need to calibrate. We think users should think about what they want to calibrate before they actually do it. This would be possible using our main simulation tool, the FMU_API, as well (https://fmi-standard.org/). I apologize that we made this first example for Dymola users only. We will update it directly. Regarding our Simulation-APIs, note from the paper:

Currently, the package supports the calibration of Functional Mock-up Units (FMUs) based on the Functional Mock-up Interface (FMI) standard (Modelica Association Project, 2021) as well as Modelica models through the python_interfaceof the Software Dymola (Dassault Systems, 2021).

So, the review of code and software should be possible using the FMU_API. If the presence of the DymolaAPI hinders the publication process, we can remove this API from the source code.

All other examples and most importantly the calibration work with the open FMI standard using the interface to fmpy. We provide two example models. One can be executed on both Linux and windows (Example B) and one only on windows (Example A). If you have trouble running these examples please let me know. I will also update the examples README.md to clarify the points I just made.

Sorry again for any inconvenience due to the bad example description. We hope you will still consider the software for publication after this clarification and the update of the example documentation.

fraukewiese commented 2 years ago

@FWuellhorst Thank you for the explanation. @ralphevins I agree that it must be possible to review and use the library/software without a license. Do you think that can be assured after the changes made by FWuellhorst?

fraukewiese commented 2 years ago

@ralphevins @samanmostafavi Could you update us on how the review is going? Thank you.

ralphevins commented 2 years ago

@fraukewiese I had my post-doc @shamsiharis take a look and he's much better qualified than me to complete the review. Can you reassign to him? He should be able to get it completed pretty quickly as he's already seen the codebase. Thanks!

fraukewiese commented 2 years ago

@whedon add @shamsiharis as reviewer

whedon commented 2 years ago

OK, @shamsiharis is now a reviewer

fraukewiese commented 2 years ago

@whedon remove @ralphevins as reviewer

whedon commented 2 years ago

OK, @ralphevins is no longer a reviewer

fraukewiese commented 2 years ago

@ralphevins Thank you for the suggestion and @shamsiharis Thank you for agreeing to review! I have assigned you as a reviewer and I have edited the checklist, so that you can start the review.

fraukewiese commented 2 years ago

If there are any questions, please let me know.

shamsiharis commented 2 years ago

@FWuellhorst I am almost done with the review. I was able to follow the tutorial along with the examples. I had two questions though.

  1. For the multiple class calibration in the tutorial, running the example in jupyter notebook returns a Simulation Failed Error. The multiple class calibrator runs successfully for the examples via the terminal although returns a maximum iterations reached error.
  2. Another error appears at the end of every example, which is related to the FMU_API (FMU_API: Could not terminate fmu instance: fmi2Terminate failed with status 3 (error)).
FWuellhorst commented 2 years ago

@shamsiharis : Thanks a lot that you take the time to review our code!

Regarding your questions:

  1. I will split my answer: Simulation error: Can you raise an issue with the simulation error in AixCaliBuHA? My colleagues and I don't get this error. Please state the error log. Maximum iterations reached error: This was a design choice. Most optimization algorithms determine when to stop. However, setting an explicit maximum number of iterations was a feature request by many users. If an error during calibration occurs, the current best result is saved. Thus, we raise our own error and catch it to exit the optimization gracefully. This could of course be an INFO-log. But we decided on the ERROR log to indicate that the optimization itself did not yet converge. Maybe a warning would be the best log option. I raised an issue: https://github.com/RWTH-EBC/AixCaliBuHA/issues/28

  2. Thanks for mentioning this. I also get this error. fmpy raises the Exception when calling fmu_instance.terminate(). Calling fmu_instance.freeInstance() first gives another error when terminating. After calling the commands, however, we are still able to remove the unzipped fmu and thus properly close it. I raised an issue in fmpy and will update our codebase once I get a response: https://github.com/CATIA-Systems/FMPy/issues/357

shantropy commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

shantropy commented 2 years ago

@FWuellhorst I've gone through the basics so far. This includes reviewing the paper, initial installation and testing. Here's where I stand at this point:

Comments on the paper:

Summary: Nicely written. I would recommend adding further references to other building energy modeling libraries (e.g., Modelica Buildings Library to further emphasize the importance of developing AixCaliBuHA.

Statement of Need: I get the impression that ebcpy is doing the heavy lifting when it comes to creating a common interface to do the handshake between the emulator and optimization. If so, what is the statement of need specific to AixCaliBuHA? I do understand that here, you are focusing on Building Energy Models, is this the main contribution?

One more note that I would like to make is that, as you may know, there are other gradient-free model calibration methods, e.g., EstimationPy, that are also geared towards Modelica models. I think it will be useful to mention how/if there is a plan for AixCaliBuHA to be integrated with such open source softwares (for example, it seems like you already have an API with pymoo). Having said this, I have to admit I'm not quiet sure if this is a necessary requirement in JOSS review.

Another minor issue: is there any acknowledgment of financial support necessary in this case?

Rest of the review:

The only thing left for me is to check its functionality and more importantly, the usefulness of the code. I have done some tests and I would like to proceed by opening issues but before I do that, could you please clarify my understanding of the following:

[Compatibility with other Libraries]: Initially, I was very excited by the prospect of paring your software with Modelica Buildings Library. This is specifically a very relevant and useful topic since the library, alongside the ongoing Ibpsa / project 1 will provide access to an ever increasing library of building model templates. I'm not having much success here. I followed the following principle: I substitute my fmu file but failed to load the FMU. I haven't really figured out the error yet (error). I looked at examples/model and noticed I needed to do inputs/outputs specification. Since I've failed at this stage, and upon reading 'Link between optimization and class definition', I am speculating that I would need to tailor something in 'modelica_calibration_templates', Is this the case? In general, and before I open an issue, is my expectation reasonable that AixCaliBuHA is meant to be compatible with other fmus? If so, does the compatibility depends on how the fmus are compiled? I have noticed that you have claimed that usage of 'modelica_calibration_templates' is "optional".

FWuellhorst commented 2 years ago

@samanmostafavi: Thanks a lot for the time invested in your initial review and your comments.

Please find my answers/comments below:

Summary: I agree. As the AixLib is part of project one, referring to all other libraries (IDEAS, Buildings, etc.) instead of only the AixLib is more fitting.

Statement of Need: The statement of need is the automated calibration. While developing the library AixCaliBuHA, we found that most interfaces can be reused for other research. For example, we used ebcpy for the design optimization of heat pump systems. So, ebcpy contains utilities and mostly abstract methods, such as the Optimizer and does a lot of heavy lifting regarding simulation and optimization. However, from the statement of need point of view, AixCaliBuHA does the "heavy" lifting of defining data classes, plotting, logging, and performing the automated calibration.

Further gradient based packages: I was not aware of EstimationPy, thank you for this recommendation. We can definitely think about integrating it into the Wrapper as well. For now, we integrate new methods If available methods fail to optimize certain tasks. As pymoo, scipy and dlib work quite quell, we have a limited priority to integrate new methods. But if you would like to see this feature, we can definitely integrate it jointly.

Founding: Not necessary in this case.

Compatibility with other Libraries It should definitely be compatible! The error you are receiving is because your FMU contains Strings, a case I encountered myself one month ago. The fix is already in the latest master. Try to install via pip install git+https://github.com/RWTH-EBC/ebcpy. If the error prevails, please raise an issue. A colleague was used AixCaliBuHA with the Buildings Library and EnergyPlus, so compatibility should be given.

shantropy commented 2 years ago

@FWuellhorst Thank you for the prompt response. I will try the fix you suggested and let you know soon. Per our discussion, I will focus the rest of review on the functionality and corresponding documentation of AixLib and how it does the "heavy lifting of defining data classes, plotting, logging, and performing the automated calibration". I also suggest to edit the Statement of Need to highlight this better.

FWuellhorst commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.00097 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1109/ACCESS.2020.2990567 is OK
- 10.1016/j.rser.2014.05.007 is OK
- 10.26868/25222708.2019.210992 is OK
- 10.3384/ecp21181561 is OK
- 10.1080/19401493.2013.765506 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.enconman.2021.114888 is INVALID because of 'https://doi.org/' prefix
FWuellhorst commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.00097 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1109/ACCESS.2020.2990567 is OK
- 10.1016/j.rser.2014.05.007 is OK
- 10.26868/25222708.2019.210992 is OK
- 10.3384/ecp21181561 is OK
- 10.1016/j.enconman.2021.114888 is OK
- 10.1080/19401493.2013.765506 is OK

MISSING DOIs

- None

INVALID DOIs

- None
FWuellhorst commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

FWuellhorst commented 2 years ago

@samanmostafavi : I updated the references and the statement of need. During rebuttal, your point regarding ebcpy became more clear to me. The old formulation "While implementing AixCaliBuHA, we identified a secondary need." is a little misleading. All features of ebcpy are currently only here because of the main need for the automated calibration. For a long time, ebcpy did not exist and AixCaliBuHA was the only repository. However, the classes in ebcpy may and already are used for other research needs. Therefore, we decided to decouple those functions from AixCaliBuHA. So, the research need is the automated calibration and both frameworks do their part of the "heavy lifting". We just put part of the contribution for sake of re-usability into a second framework (ebcpy). I hope this statement clarifies your concerns. Thanks again for your time to review AixCaliBuHA.

fraukewiese commented 2 years ago

@samanmostafavi Do the answers by FWuellhorst clarify your concerns?

fraukewiese commented 2 years ago

@FWuellhorst : Is the problem with the error @shamsiharis has mentioned solved?

FWuellhorst commented 2 years ago

@fraukewiese @shamsiharis: I fixed the issue regarding MaxIterationsReached. To test this, please re-install via pip install git+https://github.com/RWTH-EBC/AixCaliBuHA. Regarding the simulation error, I never received any logs or stats to reproduce it. Hence, I can't fix it.

shantropy commented 2 years ago

@fraukewiese As far as those questions are concerned yes. @FWuellhorst your updates look good to me. As far as the rest of the review goes, I am testing the software. I plan to finish my review soon. Thank you both for your patience.

shamsiharis commented 2 years ago

@FWuellhorst Errors have been fixed. The examples all run smoothly now.

I keep getting just this small error in the Tutorial under Advanced Calibration>> Multiple classes calibration.

ERROR FMU_API: Error: Scalar system is always singular, it may be possible to evaluate parameters to avoid this, for valve.Kv = ((if valve.LinearCharacteristic then valve.kv0+(1-valve.kv0)valve.yLim/valve.y1 else valve.kv0exp(log(1/valve.kv0)*valve.yLim/valve.y1)))/(1.0/valve.Kv1) = 1/0

Overall Review: AixCaliBuHA certainly provides a functional platform to facilitate the calibration of dynamic building models. One of the highlights of the package is the use of the FMI standards to support a software-independent implementation. This can go a long way when analyzing time-series formulation of building dynamics.

The authors have included a sufficiently detailed summary highlighting the main aspects of AixCaliBuHA. I would recommend the authors to include a broad picture of the scope of AixCaliBuHA (perhaps mention this in the statement of need). Also, the summary could be improved in terms of usability over different open source simulation environments.

The tutorial runs smoothly and explains the overall process workflow. Other examples satisfactorily explain the sensitivity analysis and calibration implementations. The descriptions could be made more explicit though in terms of real-world analysis problems.

FWuellhorst commented 2 years ago

@shamsiharis : Thanks again for the review, the comments, and the time associated with this. The error now should be fixed, see https://github.com/RWTH-EBC/AixCaliBuHA/issues/30. Regarding your overall review, I created an issue and will update the paper slightly to indicate that the main focus of the toolchain lies in dynamic simulations of HVAC and buildings. One question regarding your comment:

Also, the summary could be improved in terms of usability over different open source simulation environments.

What simulation environments do you mean? AixCaliBuHA aims at calibration, not simulation. If there is a new simulation environment that wants to use AixCaliBuHA, a SimulationAPI child-class could be written to enable said simulation environment. I could clarify this point in the paper if need, or did I understood your comment the wrong way?

fraukewiese commented 2 years ago

@fraukewiese As far as those questions are concerned yes. @FWuellhorst your updates look good to me. As far as the rest of the review goes, I am testing the software. I plan to finish my review soon. Thank you both for your patience.

Thanks for the update @samanmostafavi

fraukewiese commented 2 years ago

@FWuellhorst Errors have been fixed. The examples all run smoothly now.

I keep getting just this small error in the Tutorial under Advanced Calibration>> Multiple classes calibration.

ERROR FMU_API: Error: Scalar system is always singular, it may be possible to evaluate parameters to avoid this, for valve.Kv = ((if valve.LinearCharacteristic then valve.kv0+(1-valve.kv0)_valve.yLim/valve.y1 else valve.kv0_exp(log(1/valve.kv0)*valve.yLim/valve.y1)))/(1.0/valve.Kv1) = 1/0

Overall Review: AixCaliBuHA certainly provides a functional platform to facilitate the calibration of dynamic building models. One of the highlights of the package is the use of the FMI standards to support a software-independent implementation. This can go a long way when analyzing time-series formulation of building dynamics.

The authors have included a sufficiently detailed summary highlighting the main aspects of AixCaliBuHA. I would recommend the authors to include a broad picture of the scope of AixCaliBuHA (perhaps mention this in the statement of need). Also, the summary could be improved in terms of usability over different open source simulation environments.

The tutorial runs smoothly and explains the overall process workflow. Other examples satisfactorily explain the sensitivity analysis and calibration implementations. The descriptions could be made more explicit though in terms of real-world analysis problems.

Thanks a lot for the review @shamsiharis

fraukewiese commented 2 years ago

@samanmostafavi : Could you give us an update on your review? That would be great, thanks a lot.

shamsiharis commented 2 years ago

What simulation environments do you mean? AixCaliBuHA aims at calibration, not simulation. If there is a new simulation environment that wants to use AixCaliBuHA, a SimulationAPI child-class could be written to enable said simulation environment. I could clarify this point in the paper if need, or did I understood your comment the wrong way?

@FWuellhorst I apologize I completely missed your comment.

Yeah! You understood it correctly. It is not a major comment. You can clarify this in the paper.

I definitely look forward to using the package in my future research.

@fraukewiese Most welcome. I really enjoyed reviewing the package.

fraukewiese commented 2 years ago

Thanks you very much for the thorough review @shamsiharis . We will now wait for the further review of @samanmostafavi - are there any open questions you would like to be adressed?

shantropy commented 2 years ago

@FWuellhorst I have tested the software. Here's my review:

Software documentation:

I think this page can be significantly improved by adding more images (results, graph demonstrating workflow, etc.) and explaining the methods of choice, even if the majority of it will be done by citing other works. In addition, the dataframe, relationship to ebpcy, all the different knobs for SA and calibration etc. needs to be explained. I believe most of this already exists in the Jupyter Notebook example and the accompanying paper.

I have one further question regarding the paper: Do the authors describe how this software compares to other commonly-used packages? I think it is kinda there but not in explicit terms.

Examples:

KUDOS! I really like how clean (and fool-proof) this part is. A more detailed analysis is provided in comments on the tutorial ( which is basically the same example in a notebook format). I only have a single complaint here: I think there needs to be a "useful" example for a relatively big model that would showcase the relevance of SA+Calibration workflow. For example you can show how calibration will improve. (my two cents would be to pick something from Modelica Buildings Library/IDEAS).

Comments on the tutorial:

Summary: Nice step by step tutorial. Basics are adequately laid out. The existing workflow in AixCaliBuHA is not intuitive to the untrained eye. Having said that, once the user is patient enough to follow the instructions, I found no flaws whatsoever with the workflow. Full scores for that!

On Sensitivity Analysis Sobol sensitivity analysis will not scale for models with large number of parameters (too many samples and model evaluation are required). I also suspect this might be the case for Morris method. The authors should at least acknowledge this. Any suggestion on how to deal with such cases? Any prospect for parallelization for example? Finally, GSA methods has limitation, specially when there is inter-correlations between model parameters which will likely be the case in complex models. In these scenarios, prior assumption about models might greatly assist with better SA. Unless I am missing something here, AixCaliBuHA does not have this capability. Please confirm if otherwise.

On Calibration Very nice visualization! I generally found the plots well done and very useful. I am not convinced with black box optimization but I do understand the limitations of calibrating an FMU (which is effectively a black-box). It is likely that gradient-free methods will by and large not work well for large complex model.

Future work suggestion: While not necessary the scope of this software, I strongly encourage the authors to consider future workflows that include parsing equations out of Modelica models (xml files) (e.g., pymoca) that enables use of automatic differentiation for gradient-based optimization.

Final thought (and concern):

The software can potentially be a useful package for certain black-box optimization tasks. The tool will specially be very useful for analyzing and calibrating a growing family of small(ish) Modelica buildings models. I reserve my judgment on whether or not this will also be the case for over parameterized complex buildings models.

fraukewiese commented 2 years ago

Thanks a lot @samanmostafavi for the thorough review. @FWuellhorst : Please provide replies to the points raised by @samanmostafavi

FWuellhorst commented 2 years ago

@samanmostafavi and @shamsiharis: First of all, thank you both for your review, your comments, and the time associated with this!

@shamsiharis: Thanks, I will clarify this in the paper.

@samanmostafavi:

Documentation

I agree, I updated the page (and thus also the README.md).

Paper

Commercial software for calibration (mainly provided inside Dymola) is not highlighted, as they are not open-source. I added existing options for calibration of FMUs and explained how AixCaliBuHA may integrate them.

Examples

Thanks for the kudos. Regarding the detailed example: We are currently in the process of performing a complex calibration of a hydraulic system based on IBPSA models. This will highlight the usefulness of the sensitivity analysis. Afterward, we will add this complex example to the existing examples as well. The current example at least shows how less sensitive parameters are eliminated and thus reduce the optimization effort. If required, we can extend this by adding further tuner parameters prior to SA.

Furthermore, we encourage any users to add their use-cases to the list of examples.

Tutorial

Summary

It's good to hear that the workflow is clear through the tutorials, thanks for the comment.

Sensitivity Analysis

Regarding scaling: Parallelization will be added in future versions for the GSA. As our SimulationAPI is already enabled for parallelization, this is a quick feature. Regarding limits of GSA: AixCaliBuHA does not aim to improve GSA methods but rather uses them. The same goes for the calibration (see below). I will highlight this in the paper. Regarding suggestions for better model assumptions, do you know a software that is capable of doing this? We are not aware of existing solutions. If there is one, we will happily couple it with AixCaliBuHA.

Calibration:

Thanks!

Regarding black-box optimization: Coupled HVAC and building systems are quite complex, including nonlinear controls with state-events as well as stiff equations leading to MINLPs. For these systems, we had limited success using export to enable gradient-based methods using the Optimica compiler. We will try out pymoca in future studies as well. However, even after export, gradient-based methods for global optimization of MINLP require substantial knowledge in numerical optimization. In contrast, black-box methods may not get the global optimum, but achieve promising results with little knowledge of the system. I fully agree that researchers should first try export and gradient-based methods. However, if this fails, AixCaliBuHA will help to at least improve the calibration quality.

Regarding the Future work suggestion: If pymoca works for example models, we will include an option to automatically export the models to pymoca and link them to gradient-based calibration toolchains.

Final thought (and concern)

The usefulness in over-parameterized complex buildings models depends mostly on the optimization algorithm. As the number of options is easy to extend, AixCaliBuHA can adapt to possibly challenging calibration tasks. We used the toolchain for complex models (HighOrder Building and detailed HVAC components) already with success. If two tuner parameters exist that influence the outputs in opposite directions (e.g. internal gains and wall insulation), the expert definitely has to set useful boundaries for these optimization variables. As this is in line with your concern regarding GSA and calibration, I will empathize in the paper, that AixCaliBuHA does not substitute expert knowledge. It helps experts by automating tedious and time-consuming tasks.

@fraukewiese, @samanmostafavi, @shamsiharis : I added all requested changes into the existing PR https://github.com/RWTH-EBC/AixCaliBuHA/pull/33. If the changes are accepted, I will happily merge this PR.

shantropy commented 2 years ago

@fraukewiese I think my concerns are addressed. Both package and documentation are in a good state. My only remaining concern is that the toolchain might not be as useful for bigger problems. Having said that, and as elaborated in the response above by @FWuellhorst, the case by case, or rather a blanket, usefulness is out of the scope of this software and the developing team have done a reasonably good job with good software engineering practices that I think, and hope, will come to the aid of modelers in the near future.

@FWuellhorst It has been a pleasure reviewing your tool. I wish you the best and hope you keep up the good work in developing tools that make building modelers lives easier! I will definitely be following :-)

Cheers, Saman

P.S. @FWuellhorst yes, parsing tools such as pymoca will likely have very little chance of being applicable to complex hybrid models with thousands of equations. Having played around with it over the past few weeks, I think it's not worth the trouble for this domain of problems.