Skip to content

Fix grid deformation related bugs + other minor issues.#1126

Merged
pcarruscag merged 9 commits intodevelopfrom
fix_debug_issues
Dec 10, 2020
Merged

Fix grid deformation related bugs + other minor issues.#1126
pcarruscag merged 9 commits intodevelopfrom
fix_debug_issues

Conversation

@pcarruscag
Copy link
Copy Markdown
Member

Proposed Changes

Fix the output of FFD paraview files;
Maybe fix #1123 (@TobiKattmann give it a try if you can)
Deprecate VISUALIZE_VOLUME/SURFACE_DEF options for the reasons in #942;
Implements "HOLD_GRID_FIXED" for CMeshSolver.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

@TobiKattmann
Copy link
Copy Markdown
Contributor

Thanks for giving it a try but on both machines I get a segfault at roughly the same spot (following the screen output). Unfortunately the stack traces I get are somewhat useless (I literally get ?? on my private machine) and differ between machines so I cannot really say exactly where. At least the the point of failure on both machines is consistent ... that's really weird that the debugger "bugs out" here

Tested with your this branch on coupled_cht/incomp_2d

@pcarruscag
Copy link
Copy Markdown
Member Author

Hmmm can you run ldd --version and let me know the version number.

@TobiKattmann
Copy link
Copy Markdown
Contributor

TobiKattmann commented Dec 7, 2020

Hmmm can you run ldd --version and let me know the version number.

On my private machine with it is 2.32 and on the cluster 2.17. I get ?? with both.
I double checked that I compiled with debug flags

@pcarruscag
Copy link
Copy Markdown
Member Author

But that case is not discrete adjoint, are you running SU2_CFD or SU2_CFD_AD?

@TobiKattmann
Copy link
Copy Markdown
Contributor

I am running SU2_CFD_AD. SU2_CFD runs fine. For completeness: the case is also this tutorial case

@pcarruscag
Copy link
Copy Markdown
Member Author

Ok, I replicated the problem in the meantime, can you try a debug_optimized build (that seems to work for me regardless of the algorithm).

@TobiKattmann
Copy link
Copy Markdown
Contributor

Yep that works!

@pcarruscag
Copy link
Copy Markdown
Member Author

pcarruscag commented Dec 7, 2020

I guess I fixed it, somehow the two "DonorInfo" classes in different cpp files got mixed up.

EDIT: Rightly so apparently: https://stackoverflow.com/questions/10671956/same-class-name-in-different-c-files
Learn something new every day.

Copy link
Copy Markdown
Contributor

@WallyMaier WallyMaier 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 continuing to improve the code @pcarruscag.

This looks good to me

Copy link
Copy Markdown
Contributor

@TobiKattmann TobiKattmann 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 fixes. I have a few minor comments

Comment on lines 141 to 133

/*--- Find k closest points (need to define the comparator inline or debug build give wrong results). ---*/
/*--- Find k closest points. ---*/
partial_sort(donorInfo.begin(), donorInfo.begin()+nDonor, donorInfo.end(),
[](const DonorInfo& a, const DonorInfo& b) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok so here it took the struct DonorInfo from CIsoparametric.cpp as it is "first come first serve" kind-of rule used by the linker. Now you moved both structs into the class-namespaces where they cant start no trouble. Is it important that they are private?
Oh, and if I had any big bucks to spare i would surely send some over 💸 😉

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No the main thing is the namespace, an anonymous namespace in the cpp files would work as well.

}
}

/*--- If the gradient of the objective function is 0 so are the adjoint variables. ---*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dont run the MZ adjoint if ||del(OF)|| = 0 do you mean ||der(OF)|| in the commit message for derivative?

As this is a sanity check before dumping computational resources into a useless computation would it make sense here to write an output message?

And I dont really understand this check so I will ask later in the dev-meeting

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For completeness here, because the RMS residual is computed in "Iterate" it serves as a norm of the objective function gradient when that is what is extracted by "Iterate" for a future time-domain implementation this may need to change.

The motivation for this change was that running the adjoint with tiny values of OF gradient actually caused it to diverge.

@TobiKattmann
Copy link
Copy Markdown
Contributor

I looked over the last added commit and I would be happy to have this merged soon to get those juicy debug runs of SU2_CFD_AD with multizone cases.
Thanks again Pedro

@pcarruscag pcarruscag merged commit 10e8b27 into develop Dec 10, 2020
@pcarruscag pcarruscag deleted the fix_debug_issues branch December 10, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants