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

Consider the derivatives at boundaries in the 2D-table #3893

Merged
merged 15 commits into from
Feb 16, 2023

Conversation

HansOlsson
Copy link
Contributor

@HansOlsson HansOlsson commented Nov 9, 2021

Issue: That is best described using the new test-model plot input and output of the derivative-blocks and see that the inputs don't change with time, but still half of them have spikes in their outputs (*).

Solution: If we are at boundaries between regions in the 2D-table we should ideally chose the branch we will be at in the future - or the one we were at in the past.

Detailed explanation:
For a 2D-table the problem is that we can be at a double boundary and the solution e.g., goes C->B, but in the middle we could select A or D, and in those regions the derivatives differ for linear interpolation.

A B
C D

This also explains why I ignored it for the 1D-tables; as we cannot be in such a point between four regions in a 1D-table.

*: All four variants now have the same issue. They didn't originally.

It might need a better explanation, better motivating example, and the actual code could be made less impactful.

@HansOlsson HansOlsson added L: Blocks Issue addresses Modelica.Blocks L: C-Sources Issue addresses Modelica/Resources/C-Sources labels Nov 9, 2021
@HansOlsson
Copy link
Contributor Author

HansOlsson commented Nov 9, 2021

The example is now fully updated; and it turns out that there were more subtle differences for the cases.

The comment explains it:

  • Case 2 and 4 slide diagonally (from different corners)
  • Case 1 and 3 start at the edge of the real table and then leave it top-left - Interpolating in different ways

@beutlich beutlich self-assigned this Nov 11, 2021
@beutlich beutlich changed the title Consider the derivatives at boundaries in the 2D-table. Consider the derivatives at boundaries in the 2D-table Nov 12, 2021
@HansOlsson HansOlsson marked this pull request as ready for review November 15, 2021 10:02
@HansOlsson
Copy link
Contributor Author

HansOlsson commented Jan 31, 2022

Typo on line 505: /* Same as findRowIndex2 but works on rows */

should be /* Same as findRowIndex2 but works on columns */

Just to be clear: That is an existing issue, I just blindly copy-pasted. (But it is now corrected.)

/* Same as findRowIndex but works on rows */

@beutlich
Copy link
Member

@HansOlsson Will you rebase your branch to resolve the conficts or shall I?

@HansOlsson
Copy link
Contributor Author

@HansOlsson Will you rebase your branch to resolve the conficts or shall I?

I can do it.

ModelicaTest/Tables/CombiTable2Ds.mo Outdated Show resolved Hide resolved
@beutlich
Copy link
Member

beutlich commented Feb 1, 2022

@HansOlsson I added some minor formatting corrections and hope you agree on.

We should squash the commits anyway before/during the merge.

@HansOlsson HansOlsson requested a review from beutlich March 9, 2022 12:38
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

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

@HansOlsson I added some missing cases (to have it fully symmetric between ModelicaStandardTables_CombiTable2D_getDerValue and ModelicaStandardTables_CombiTable2D_getDer2Value). Feel free to reset if you skipped these cases intentionally.

We should also squash all commits before the merge.

@beutlich beutlich added this to the MSL4.1.0 milestone Mar 15, 2022
@beutlich beutlich requested a review from sjoelund March 15, 2022 20:00
@HansOlsson
Copy link
Contributor Author

@MartinOtter @sjoelund this PR has been stuck for about 2 months.

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 L: C-Sources Issue addresses Modelica/Resources/C-Sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants