-
Notifications
You must be signed in to change notification settings - Fork 847
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
Heat solver fixes for primal and adjoint CHT simulations #1107
Conversation
if (implicit) { | ||
|
||
Jacobian_i[0][0] = -thermal_diffusivity*Area; | ||
Jacobian.SubtractBlock2Diag(iPoint, Jacobian_i); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sign, I also moved this so that it only applies to one of the coupling modes, to be consistent with the implementation of the heat flux wall.
auto iter = System.Solve(Jacobian, LinSysRes, LinSysSol, geometry, config); | ||
SetIterLinSolver(iter); | ||
SetResLinSolver(System.GetResidual()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, it also fixes the output of the linear solver stuff.
@TobiKattmann since we were talking about it yesterday, the advantage of the "weak" formulation is that it is conservative, heat fluxin equal heat flux out (because the post processed fluxes are consistent with how the boundary condition is applied). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcarruscag Thanks for the contribution!
I looked the files changes and it looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks pedro, I did not test the jacobian influence yet but will so do so soon.
(This Approve excludes my bits in here of course as that is not exactly the idea of a PR-review :D )
* \param[in] solver - The solver container holding all terms of the solution. | ||
* \param[in] config - Definition of the particular problem. | ||
*/ | ||
void SetSensitivity(CGeometry *geometry, CSolver **solver, CConfig *config) override; | ||
void SetSensitivity(CGeometry *geometry, CConfig *config, CSolver*) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took me a second to figure out what happens here with the override function in the child and the default argument in the parent class. Kind of odd that the override function understands that it has to use the default argument of the parent implementation... I hope I understood that correctly (I used Compiler Explorer to make a sample)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can think of it as the default value being part of the function signature, the signature is defined by the original virtual function which is all a caller needs to know (you include CIteration.hpp to use all the other iterations), the default value is passed when calling, not used when "receiving" the call.
|
||
// Other surface-related output values | ||
// The names are different than in CConfig... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you mean that the Output has a different name for an Objective than the map Objective_Map
in option_structure provides? thats why you cannot simply use the name of the objective_function that is stored in the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... those name choices were not very good, it needs to be fixed at some point, but it will have implications for the python stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes that historyMap.py
... maybe automatically create that idk? But that is something for another day/PR :D
* \return Kind of objective function. | ||
* \brief Similar to GetKind_ObjFunc but returns the corresponding string. | ||
*/ | ||
unsigned short GetKind_ObjFunc(unsigned short val_obj) const { return Kind_ObjFunc[val_obj]; } | ||
string GetName_ObjFunc(unsigned short val_obj = 0) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for completeness can we get the param[in] and return here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I just find that kind of description too much fluff, one of my pet peeves with this doxy stuff is that a ton of methods describe the same arguments over and over ad nauseam and they say very little of the actual method. Less is more is what I'm getting at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
* \brief A virtual member.
"No **** Sherlock, I could see that from just the code..." The important is what the method should do.
Anyway, rant over.
Merging this to allow all tests to run so we can create a release tomorrow (there is a fix in this branch for compilation with gcc 5.3.0 so this should be part of the release). Happy to make post mortem changes if needed. |
Proposed Changes
I think one sign was flipped in the Jacobian of the CHT boundary.
I replaced the isothermal BC for a stronger(?) alternative (flying by the seat of my pants here, if it's wrong let me know, but the solver converges better like this).
Fixes issues with deforming heat domains by generalising CDiscAdjMeshSolver::SetSensitivity to accept any solver as "target" (instead of considering always the ADJFLOW_SOL position in the solver container).
Related Work
#951
#1061
(third time lucky I hope...)
PR Checklist