Skip to content

Add initial data in watch integrals and watch points#2181

Merged
BenjaminRodenberg merged 6 commits intoprecice:developfrom
BenjaminRodenberg:export-watchpoint-initial-data
Feb 21, 2025
Merged

Add initial data in watch integrals and watch points#2181
BenjaminRodenberg merged 6 commits intoprecice:developfrom
BenjaminRodenberg:export-watchpoint-initial-data

Conversation

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor

@BenjaminRodenberg BenjaminRodenberg commented Jan 24, 2025 •
edited
Loading

Main changes of this PR

Closes #2180

Motivation and additional information

See #2180

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I stuck to C++17 features.
  • I stuck to CMake version 3.22.1.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@BenjaminRodenberg BenjaminRodenberg added the enhancement A new feature, a new functionality of preCICE (from user perspective) label Jan 24, 2025
@BenjaminRodenberg BenjaminRodenberg added this to the Version 3.2.0 milestone Jan 24, 2025
@BenjaminRodenberg BenjaminRodenberg self-assigned this Jan 24, 2025
@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Jan 24, 2025 •
edited
Loading

Is there any good reason to not always include the initial data?

I would consider this a bugfix. @uekerman what do you think?

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Jan 24, 2025 •
edited
Loading

Btw: Tests are failing because all watchpoints now include the initial data and this does not match the data the test expects.

Correction: WatchIntegralScaleAndNoScale fails. From my perspective we could also only add the initial data to the watchpoints and skip the watch integrals. I only applied the change consistently in this PR because it looked like the right thing to do. But one might also argue this is out of scope for a PR that is dealing with watchpoints.

For me this is a bit hard to judge since I do not use the watch integrals.

@BenjaminRodenberg BenjaminRodenberg force-pushed the export-watchpoint-initial-data branch from df4ce67 to 2dd21ad Compare January 24, 2025 17:19
@BenjaminRodenberg BenjaminRodenberg marked this pull request as ready for review January 24, 2025 17:20
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Jan 24, 2025

I removed quite some code from the original PR:

  • 0bd3a42 removes the code influencing the watch integrals. Then we do not have to update any tests.
  • 2dd21ad removes the code for making it configurable. Assuming we consider this as a bugfix and non-breaking.

Let's try to fix the scope of this PR next. Then we can also revert these commits and add the required code again if necessary.

@BenjaminRodenberg BenjaminRodenberg added bug preCICE does not behave the way we want and we should look into it (and fix it if possible) and removed enhancement A new feature, a new functionality of preCICE (from user perspective) labels Jan 24, 2025
This was referenced Jan 24, 2025
Closed
Closed
@BenjaminRodenberg BenjaminRodenberg added enhancement A new feature, a new functionality of preCICE (from user perspective) and removed bug preCICE does not behave the way we want and we should look into it (and fix it if possible) labels Jan 27, 2025
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Jan 27, 2025

@MakisH added you as a reviewer after our discussion in the telco. As far as I understood we are putting this PR on hold until some other issues are resolved. Could you maybe provide pointers to these issues here and review/merge as soon as everything looks good from your perspective?

@MakisH
Copy link
Copy Markdown
Member

MakisH commented Jan 27, 2025

I agree that this is a nice addition. What I meant in the telco: let's wait before merging this (and #2132) until we conclude on the regression encountered in #2052 and update the reference results, to not run after multiple regressions at the same time.

I think there is not much left to do there, I will write in both PRs once we can go ahead.

@MakisH MakisH mentioned this pull request Jan 27, 2025
Merged
3 tasks
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Jan 27, 2025
Open
fsimonis
fsimonis reviewed Jan 29, 2025
View reviewed changes
BenjaminRodenberg
BenjaminRodenberg commented Jan 30, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

I added the watch integrals. I'm a bit surprised by the results in the test. As mentioned earlier: I do not work with watch integrals so there might be some misunderstanding. The 3.0 in the last column surprises me generally: I thought the last column is the are of the surface which should be 1.0 for the given triangle.

@BenjaminRodenberg BenjaminRodenberg changed the title Add initial data in watchpoints Add initial data in watch integrals and watch points Jan 31, 2025
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Jan 31, 2025 •
edited
Loading

At the moment we can either merge #2188 or #2181 first. After merging one of the two PRs we will potentially need to update some tests in the other one.

@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Jan 31, 2025
Merged
9 tasks
@fsimonis fsimonis force-pushed the export-watchpoint-initial-data branch from e13481a to b80b17b Compare February 3, 2025 15:06
@BenjaminRodenberg BenjaminRodenberg added the help wanted I have opened this issue and/or I am assigned to it, but I don't know how to solve it label Feb 3, 2025
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Feb 3, 2025

Added the help wanted label to indicate that merging this PR currently depends on another one. We could also consider introducing a new label stuck or waiting for this purpose.

@BenjaminRodenberg BenjaminRodenberg removed the help wanted I have opened this issue and/or I am assigned to it, but I don't know how to solve it label Feb 21, 2025
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Feb 21, 2025

Not that #2052 is merged I will merge this PR.

@BenjaminRodenberg BenjaminRodenberg merged commit cf04574 into precice:develop Feb 21, 2025
@MakisH
Copy link
Copy Markdown
Member

MakisH commented Feb 21, 2025

FYI, in the end this did not conflict with the reference results already used by the system tests: https://github.com/precice/tutorials/actions/runs/13460800713

@fsimonis fsimonis deleted the export-watchpoint-initial-data branch February 22, 2025 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fsimonis fsimonis fsimonis left review comments

@MakisH MakisH Awaiting requested review from MakisH

Assignees

@BenjaminRodenberg BenjaminRodenberg

Labels

enhancement A new feature, a new functionality of preCICE (from user perspective)

Projects

None yet

Milestone

Version 3.2.0

Development

Successfully merging this pull request may close these issues.

Export initial data in watchpoint

3 participants

@BenjaminRodenberg @fsimonis @MakisH