Skip to content

Intermediate base class for flow solvers to avoid duplication. #1044

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.

Sign up for GitHub

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

Jump to bottom
Merged
pcarruscag merged 23 commits into from
Jul 22, 2020
Merged

Intermediate base class for flow solvers to avoid duplication. #1044

pcarruscag merged 23 commits into from
Jul 22, 2020

Conversation

@pcarruscag
Copy link
Member

@pcarruscag pcarruscag commented Jul 8, 2020

Proposed Changes

As discussed in today's developers meeting.

Related Work

#1014

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).
  • 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.

pcarruscag
pcarruscag commented Jul 8, 2020
View reviewed changes
SU2_CFD/include/solvers/CEulerSolver.hpp
Comment on lines 38 to +39
* \author F. Palacios
*/
class CEulerSolver : public CSolver {
class CEulerSolver : public CFVMFlowSolverBase<CEulerVariable, COMPRESSIBLE> {
Copy link
Member Author

@pcarruscag pcarruscag Jul 8, 2020

Choose a reason for hiding this comment

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

Exhibit A

SU2_CFD/src/solvers/CEulerSolver.cpp
Comment on lines 210 to 149
ReducerStrategy = parallelEff < COLORING_EFF_THRESH;
Allocate(*config);
Copy link
Member Author

@pcarruscag pcarruscag Jul 8, 2020

Choose a reason for hiding this comment

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

Exhibit B

SU2_CFD/src/solvers/CNSSolver.cpp
Comment on lines +32 to +36
#include "../../include/solvers/CFVMFlowSolverBase.inl"

/*--- Explicit instantiation of the parent class of CEulerSolver,
* to spread the compilation over two cpp files. ---*/
template class CFVMFlowSolverBase<CEulerVariable, COMPRESSIBLE>;
Copy link
Member Author

@pcarruscag pcarruscag Jul 8, 2020

Choose a reason for hiding this comment

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

Exhibit C

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp Outdated
Comment on lines 34 to 35
template<class VariableType, ENUM_REGIME FlowRegime>
class CFVMFlowSolverBase : public CSolver {
Copy link
Member Author

@pcarruscag pcarruscag Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Declarations and inline function in the hpp file.
The first template parameter is the type for nodes this is so we can still have de-virtualization.

SU2_CFD/include/solvers/CFVMFlowSolverBase.inl Outdated
Comment on lines 61 to 63
template<class V, ENUM_REGIME R>
void CFVMFlowSolverBase<V,R>::Allocate(const CConfig& config) {

Copy link
Member Author

@pcarruscag pcarruscag Jul 8, 2020

Choose a reason for hiding this comment

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

Implementations of long functions go to the .inl file, we include this file in the cpp where we want the instantiation of the class template to take place (we can spread out the compilation cost like this).

@pcarruscag pcarruscag mentioned this pull request Jul 9, 2020
Closed
@EttoreSaetta EttoreSaetta mentioned this pull request Jul 9, 2020
Merged
3 tasks
@pcarruscag
Copy link
Member Author

pcarruscag commented Jul 15, 2020

@koodlyakshay can you have a quick look if this will cause too much trouble for the pressure based solver?

@pcarruscag pcarruscag mentioned this pull request Jul 18, 2020
Closed
5 tasks
@pcarruscag
Copy link
Member Author

pcarruscag commented Jul 18, 2020

Can we please merge this darn thing already? This was discussed and presented in 2 dev meetings, and people are starting to fix problems that are already fixed here, I do not like wasting time fixing pointless merge conflicts.

talbring
talbring approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks @pcarruscag ! To me that looks good.

CatarinaGarbacz
CatarinaGarbacz approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@CatarinaGarbacz CatarinaGarbacz left a comment

Choose a reason for hiding this comment

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

Looks good and solid as a first step towards this new inheritance structure for flow solvers. thanks @pcarruscag , NEMO will make good use of this

@pcarruscag
Copy link
Member Author

pcarruscag commented Jul 22, 2020

Thank you both, now there will be precedent for less duplication :)

@pcarruscag pcarruscag merged commit 16f3836 into develop Jul 22, 2020
@pcarruscag pcarruscag deleted the cleanup_flow_solver_duplication branch July 22, 2020 16:28
@pcarruscag pcarruscag mentioned this pull request Jan 23, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@talbring talbring talbring approved these changes

@CatarinaGarbacz CatarinaGarbacz CatarinaGarbacz approved these changes

Assignees

No one assigned

Labels

changelog:chore PR: merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pcarruscag @talbring @CatarinaGarbacz