search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
452 stars 165 forks source link

Restore checks for extrapolation detection of CombiTable2D #4360

Closed beutlich closed 3 weeks ago

beutlich commented 3 months ago

This restores the checks for extrapolation detection in functions ModelicaStandardTables_CombiTable2D_getDerValue and ModelicaStandardTables_CombiTable2D_getDer2Value as regression of #3893. They are also consistent with ModelicaStandardTables_CombiTable2D_getValue where there is no derivative information.

Closes #4343.

This also is relevant for MSL 4.1.0: If merged, it needs to be back-ported to maint/4.1.x branch and binaries need to be rebuilt as well.

beutlich commented 3 months ago

but I don't see that this PR currently solves the issue.

Not sure what you mean exactly. Can read it as the patch is not good enough or not good at all.

I can confirm, that by restoring the extrapolation checks (to MSL v4.0.0 behaviour) the reported regression (of ModelicaTest.Tables.CombiTable2Ds.Test26 etc.) no longer is oberservable. See https://github.com/modelica/ModelicaStandardLibrary/issues/4343#issuecomment-2013739246

Edit: The patch already is applied to ModelicaTableAdditions: https://github.com/tbeu/ModelicaTableAdditions/commit/cf98e2b1c0a2dec06d4d10e875c0eea0e8b06ee6

HansOlsson commented 3 months ago

but I don't see that this PR currently solves the issue.

Not sure what you mean exactly. Can read it as the patch is not good enough or not good at all.

I'm not sure if it is good enough at least.

The problem that https://github.com/modelica/ModelicaStandardLibrary/pull/3893 intends to solve also occurs for the boundary, and as far as I can tell this PR removes that handling for the boundary. The test-case for that PR even include a test at the boundary - but it could be it doesn't cover all possibilities and thus doesn't trigger.

HansOlsson commented 3 weeks ago

As explained above I propose to close this without merging it - as I view #3893 as more problematic than switching between left- and right-limit at end of simulation.

beutlich commented 3 weeks ago

As you can imagine I am not happy with this rejection. The reason is that now the second derivative no longer corresponds to the first derivative: Jump to zero in the second derivative whereas first derivative is not constant. I consider this also a true regression (and an independent one of #4415).