Skip to content

Update Math.Vectors.interpolate #4641

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update Math.Vectors.interpolate #4641

wants to merge 2 commits into from

Conversation

DagBruck
Copy link
Contributor

Performance improvements as described in #4639.

Performance improvements as described in #4639.
@DagBruck DagBruck added this to the MSL4.2.0 milestone May 12, 2025
@DagBruck DagBruck linked an issue May 12, 2025 that may be closed by this pull request
@DagBruck DagBruck requested review from casella and HansOlsson May 12, 2025 13:30
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fairly good.
My only concern is that it seems as if the interface is underspecified.
Consider Vectors.interpolate({0, 2, 2}, {1,2,3}, 4)
It says that should extrapolate through the last two points, but since they have the same x-value it is unclear what "extrapolation" means.
The previous variant seems to have asserted on that (possibly by mistake due to left-over code), now we return 4.
I think the assert might be preferable - and that it should be documented.

@DagBruck DagBruck added the L: Math Issue addresses Modelica.Math label May 12, 2025
@DagBruck
Copy link
Contributor Author

DagBruck commented May 12, 2025

Well, the documentation is contradictory and the check appears to be inconsistent. See example in #4639 (comment). I think you get a diagnostic when the last two x-values are the same.

We can trace this discussion back to #1555, but the alleged fix did in fact not implement what was requested.

Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DagBruck would you mind adding some test cases of the specially handled cases to ModelicaTest as well?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Math Issue addresses Modelica.Math
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements to Modelica.Math.Vectors.interpolate
3 participants