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

Wrong result in Buildings.Fluid.HydronicConfigurations.PassiveNetworks.Examples.SingleMixing #10580

Open
mwetter opened this issue Apr 19, 2023 · 7 comments
Assignees

Comments

@mwetter
Copy link

mwetter commented Apr 19, 2023

Description

The model Buildings.Fluid.HydronicConfigurations.PassiveNetworks.Examples.SingleMixing (and others in this package that use similar constructs) have wrong results with OpenModelica.

The result below is from Buildings, commit 9b4be2033c1765ad5f518352ee164b8c7d78e775
The error is in the instance T2SetMod:
image

The OpenModelica results are as follows:
image

Note that index changes, but the output y does not.
It looks like the line
https://github.com/lbl-srg/modelica-buildings/blob/9b4be2033c1765ad5f518352ee164b8c7d78e775/Buildings/Controls/OBC/CDL/Routing/RealExtractor.mo#L21
is not computed properly.

Steps to Reproduce

Download Buildings, commit 9b4be2033c1765ad5f518352ee164b8c7d78e775
Simulate Buildings.Fluid.HydronicConfigurations.PassiveNetworks.Examples.SingleMixing with dassl, 1E-6.

Expected Behavior

See above.

Version and OS

  • OpenModelica Version: OpenModelica 1.22.0~dev-41-g8a5b18f
  • OS: Linux Ubuntu 20.04
@mwetter
Copy link
Author

mwetter commented Apr 19, 2023

It turns out that this work-around gives the right result:
lbl-srg/modelica-buildings@48ad1f6
image

@casella
Copy link
Contributor

casella commented Apr 19, 2023

That's kind of weird, the two formulations should be totally equivalent.

@mahge mahge self-assigned this Apr 20, 2023
rfranke added a commit to rfranke/OpenModelica that referenced this issue May 2, 2023
…for C++)

See e.g.
Buildings.Fluid.HydronicConfigurations.PassiveNetworks.Examples.SingleMixing
@rfranke
Copy link
Member

rfranke commented May 2, 2023

It works with the Cpp runtime after PR #10642. See also #10603 for another example generating different events for C and C++.

rfranke added a commit that referenced this issue May 2, 2023
See e.g.
Buildings.Fluid.HydronicConfigurations.PassiveNetworks.Examples.SingleMixing
@mahge
Copy link
Contributor

mahge commented May 25, 2023

I am not a 100% sure yet but this seems to be an issue with the removeSimpleEquations module. It appears the module constant evaluates the equation when it is in an equation section. It probably should not.

The library version in question is not available in the OpenModelica package manager (?) so I am working with a MWEs that I think are similar. It would be very helpful if you can provide a MWE for it. Otherwise I will check more and let you know.

@casella
Copy link
Contributor

casella commented May 26, 2023

@mahge I tried to come up with an MWE only containing the signal blocks, but if I do so, everything works fine. I attach an MWE package to reproduce the issue (compare SingleMixing with SingleMixingAlgorithm, which uses the tweaked block with the algorithm instead of the equation)
MWE.zip

@mahge
Copy link
Contributor

mahge commented May 26, 2023

@mahge I tried to come up with an MWE only containing the signal blocks, but if I do so, everything works fine

Yes it was annoying me as well 🙂. One thing I found that made the two (algorithm vs equation) behave different was to have index as a parameter with no binding in my MWEs. However, I was not 100% sure if that represented the actual problem here.

I will check your MWE and will let you know.

@casella
Copy link
Contributor

casella commented May 26, 2023

I will check your MWE and will let you know.

It's not such a big deal, it is basically just a copy of the original model, just set up so it depends on the installed version.

mahge added a commit to mahge/OpenModelica that referenced this issue May 31, 2023
  - I am not sure why this is done the way it is now. However, it does not
    seem like it is correct. It takes a cref, replaces all variable
    subscripts (non-constant subscripts) with whole dims, expands the now
    array type cref (because of the new wholdims) to scalars, and returns
    them for potential replacement.

    I am not sure why this has worked so far. Maybe it is complemented by
    some other mechanism somewhere else but it does not look right.

    This fixes the issue in OpenModelica#10580.
mahge added a commit to mahge/OpenModelica that referenced this issue Jul 11, 2023
  - I am not sure why this is done the way it is now. However, it does not
    seem like it is correct. It takes a cref, replaces all variable
    subscripts (non-constant subscripts) with whole dims, expands the now
    array type cref (because of the new wholdims) to scalars, and returns
    them for potential replacement.

    I am not sure why this has worked so far. Maybe it is complemented by
    some other mechanism somewhere else but it does not look right.

    This fixes the issue in OpenModelica#10580.
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

No branches or pull requests

4 participants