Skip to content

Conversation

@sebproell
Copy link
Member

This PR makes the internal node numbers equal to the node numbers specified in the dat-style or Exodus input.

Closes #731

@sebproell sebproell added type: bug report Issues reporting a bug in the code team: infrastructure labels May 2, 2025
@sebproell sebproell self-assigned this May 2, 2025
@sebproell sebproell marked this pull request as draft May 2, 2025 13:54
ischeider
ischeider previously approved these changes May 2, 2025
Copy link
Contributor

@ischeider ischeider left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! Also the whole -"1 procedure" seems more consistent across the problem types.

bgoderbauer
bgoderbauer previously approved these changes May 2, 2025
@sebproell
Copy link
Member Author

This PR reveals loads of interesting things. I suspect we depend on some implicit numbering a lot more than we would like. Some observations:

  • Some contact tests have the correct results but iteration numbers go up: https://github.com/4C-multiphysics/4C/actions/runs/14796604344/job/41548992303#step:7:483
  • Scatra multi-scale somehow still depends on zero-based nodes
  • Cardiovascular has very small deviations.
  • Specifying a node to test results on for the DOMAIN input is questionable since the node numbering is a pure implementation detail. Same holds for ParticleWall. We should test results at geometric coordinates instead.

@sebproell sebproell dismissed stale reviews from bgoderbauer and ischeider via 875882c May 2, 2025 15:49
@maxfirmbach
Copy link
Contributor

@sebproell @mayrmt I'm almost certain the problem with the rising iteration numbers in contact is related to multigrid internals. In detail how aggregates are build on the contact interface ... seems to me this is still buggy as we internally calculate dofs from nodes (the numbering matters there, I didn't thought it has such an impact though). Test still passes as the overall procedure converges, but likely internally in the preconditioning something goes wrong.

@sebproell
Copy link
Member Author

sebproell commented May 3, 2025

We should not depend on any numbering. However, there seem to be some very nasty dependencies on the zero-based numbering, and I am not sure I will be able to cleanly resolve these here. @maxfirmbach can you point me to the code where you think we do something questionable based on node numbering.

Overall, getting this right seems even more important for the future robustness of the code base, now that we see how much it impacts existing code.

@maxfirmbach
Copy link
Contributor

@sebproell This piece of code lives in Trilinos itself ... I think @PhilipOesterlePekrun is already (indirectly) working on this here trilinos/Trilinos#13859

@mayrmt
Copy link
Member

mayrmt commented May 3, 2025

@sebproell @maxfirmbach there is currently work in progress to replace the degree of freedom dependent aggregation scheme by a node based aggregation scheme. This should not depend on any conversions between dofs and nodes.

I expect this to be merged into Trilinos within the next couple of weeks

@mayrmt
Copy link
Member

mayrmt commented Aug 28, 2025

@sebproell @maxfirmbach The necessary changes to Trilinos have been merged into Trilinos in the mean time. Necessary changes to 4C are work in progress, cf. #878, but require a Trilinos update in 4C in order to be mergable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team: infrastructure type: bug report Issues reporting a bug in the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Does the exodus read feature change the results?

5 participants