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

Include SDF #1959

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Include SDF #1959

wants to merge 4 commits into from

Conversation

AntoineGautier
Copy link
Contributor

@AntoineGautier AntoineGautier commented Jan 29, 2025

This is for #1958.

TODO:

  •  Add copyright notice
  • Add some auto-generated unit tests for models in IBPSA.Utilities.IO.SDF.Examples
  • Test example models with OMC: ✅ tests pass
  • Investigate why example models fail to simulate with OCT
    • [Modelon - 3172] Support Ticket: Support for arrays that can be initialized at runtime
      Response from Modelon:

      It is currently not allowed to have the size of a parameter depend on a "impure" function. The size needs to be computed by pure functions for the model to work with OCT.
      The model might work if "impure" is removed from the functions involved.

    • Testing various refactoring of the Modelica functions to avoid invoking impure functions for assigning array sizes, the initial translation error mutates into either a C-compilation error or a Java null pointer exception. I conclude from these first tests that OCT support is not granted. This is somewhat surprising as the SDF library was initially developed by Modelon AB (see New ND-table based on SDF to be integrated in MSL modelica/ModelicaStandardLibrary#1516 (comment)).

@AntoineGautier
Copy link
Contributor Author

AntoineGautier commented Jan 29, 2025

@mwetter @FWuellhorst This draft PR shows how the SDF library could be integrated into IBPSA. The integration is done with the script https://github.com/ibpsa/modelica-ibpsa/blob/issue1958_sdf/IBPSA/Resources/src/Utilities/IO/SDF/importSDF.py

Please let me know what you think in terms of

@FWuellhorst
Copy link
Contributor

@AntoineGautier @mwetter : Nice work, glad to see SDF as an option.

  • If it is allowed license-wise (I am not a legal expert), I think the location is a good fit.
  • If we have examples using SDF in the heat pump models, testing would not necessarily be required. If SDF itself does not have testing included, and we start to use it in other models as well, some basic tests could be in order.
  • Regarding HP models, I already included options in AixLib, and we could just merge those into IBPSA. I just extended the SDF based approach a little to avoid code redundancy when using 4 or 5 dimensions and am really close to finish an experimental validation which uses 5D performance data (including compressor speed, condenser mass flow rate and degree superheat). So once SDF is supported in IBPSA, I could open that PR. Based on your needs, we could adjust the format.

@mwetter
Copy link
Contributor

mwetter commented Jan 29, 2025

@AntoineGautier @FWuellhorst

  • The package location looks good to me. I would however put the importSDF.py to Resources/src/Utilities/IO/SDF (this is what we also use in Buildings for other code that imports 3rd party code). The script is also picky regarding the location from which it is invoked.
  • The licenses are BSD, so if we reproduce them I don't see any issue
  • Adding test would be good. For example, IBPSA.Utilities.IO.SDF.Examples.Playback fails (the other two examples work)
  • We should also put a sentence in IBPSA.Utilities.IO.SDF explaining where it comes from.
  • Including this feature in the HP models would be good (and is in my view the main motivation to add SDF).

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.

3 participants