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

Print values in assertion message of VariableLimiter #4519

Closed
wants to merge 1 commit into from

Conversation

beutlich
Copy link
Member

Fixes #4482.

@beutlich beutlich added L: Blocks Issue addresses Modelica.Blocks P: trivial Trivial issue labels Jan 20, 2025
@HansOlsson
Copy link
Contributor

I would prefer if we instead encouraged tools to consistently include such information in the assert-messages, instead of manually patching every assert-call.

@beutlich
Copy link
Member Author

beutlich commented Jan 22, 2025

I would prefer if we instead encouraged tools to consistently include such information in the assert-messages, instead of manually patching every assert-call.

In that case #4482 could have been labeled tool-issue. But it was not.

Meanwhile, it does not hurt to print the values already in the assert message. I also tested in Dymola 2024 R1 where it now looks better (with the patch applied).

@henrikt-ma
Copy link
Contributor

I would prefer if we instead encouraged tools to consistently include such information in the assert-messages, instead of manually patching every assert-call.

In that case #4482 could have been labeled tool-issue. But it was not.

Meanwhile, it does not hurt to print the values already in the assert message. I also tested in Dymola 2024 R1 where it now looks better (with the patch applied).

No, we can't both encourage tools to include the information and at the same time explicitly embedding the information in the messages.

If we are serious about encouraging tools, on the other hand, these are some asserts that should be reformulated in order to not come out ugly with tools that also provide details of the failed assert condition:

        assert(uMax >= uMin, "Limiter: Limits must be consistent. However, uMax (=" + String(uMax) +
                             ") < uMin (=" + String(uMin) + ")");
        assert(uMax >= uMin, "DeadZone: Limits must be consistent. However, uMax (=" + String(uMax) +
                             ") < uMin (=" + String(uMin) + ")");
    assert(index >= 1 and index <= nin,
      "The input index (=" + String(index) + ") is out of range.");
            assert(table[i] > table[i-1],
              "Time values of table not strict monotonically increasing: table["
               + String(i - 1) + "] = " + String(table[i - 1]) + ", table[" +
              String(i) + "] = " + String(table[i]));

and many, many more.

There is also this kind of message, which doesn't embed values, but which will still not look great with a tool that displays the failed assert condition (giving context to the automatically provided values):

    assert(limit1 >= limit2, "Input signals are not consistent: limit1 < limit2");

@beutlich
Copy link
Member Author

No, we can't both encourage tools to include the information and at the same time explicitly embedding the information in the messages.

In that case #4482 could have been labeled tool-issue. But it was not.

There is also this kind of message, which doesn't embed values, but which will still not look great with a tool that displays the failed assert condition (giving context to the automatically provided values):

    assert(limit1 >= limit2, "Input signals are not consistent: limit1 < limit2");

See again. This is exactly the assert message this PR was about.

Anyway, closing.

@beutlich beutlich closed this Jan 23, 2025
@beutlich beutlich deleted the update-assert branch January 23, 2025 05:44
@beutlich beutlich added the wontfix Issue that will not be fixed label Jan 23, 2025
@beutlich beutlich added this to the never milestone Jan 23, 2025
@HansOlsson
Copy link
Contributor

See #4399 for the general issue.

HansOlsson added a commit to HansOlsson/ModelicaSpecification that referenced this pull request Jan 23, 2025
See: modelica/ModelicaStandardLibrary#4519 and modelica/ModelicaStandardLibrary#4399

The reason for "may" is:
- To not require the value to be duplicated, for cases such as: modelica/ModelicaStandardLibrary#4399
- Support even smarter debugging.

I thought it was sufficiently important to make it normative.
@beutlich
Copy link
Member Author

See #4399 for the general issue.

Thanks for reminding. Citing your comments from there:

Use more digits in the message, by using String(T, 11) or so

That's exactly what I did in this PR.

Make the assert-text even simpler (perhaps "Temperature is not in the allowed range") and leave the debugging messages to the tools ...

I consider this just as a small side note but not yet as general advice on tool-vendors to improve there debugging messages on assertions. If it would already be the case then #4482 had not been reported.

So questions are

  • update MSL in either one way or the other to have it at least no longer such inconsistent as reported by @henrikt-ma
  • improve tools independent of the decision how to adapt MSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Blocks Issue addresses Modelica.Blocks P: trivial Trivial issue wontfix Issue that will not be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be more explicit in assert of VariableLimiter
3 participants