Decouple compilation of 2D and 3D mprans classes #1140
Conversation
|
@JohanMabille can you take a look at this PR. I'm thinking I'd like to go ahead and split the RANS* classes to have separate modules following the same pattern. Mainly just to prevent confusion in the future. The compile times appeared to be slightly faster on my laptop, but the difference was marginal. This is mainly just for consistency/simplicity. |
|
Also @JohanMabille can you explain why #define FORCE_IMPORT_ARRAY is necessary? |
| @@ -10,6 +10,9 @@ | |||
| #include "ModelFactory.h" | |||
| #include "SedClosure.h" | |||
| #include "equivalent_polynomials.h" | |||
| const double DM=0.0;//1-mesh conservation and divergence, 0 - weak div(v) only | |||
| const double DM2=0.0;//1-point-wise mesh volume strong-residual, 0 - div(v) only | |||
| const double DM3=1.0;//1-point-wise divergence, 0-point-wise rate of volume change | |||
There was a problem hiding this comment.
I think these variables could be static to avoid multiple symbols definitions if this header is included in many cpp files in the same library. This is just a precaution.
There was a problem hiding this comment.
These headers really shouldn't be used in more than one compilation unit, and the point here is to make the symbols local constants. They're just switches that are specific to this file (could also be macros).
proteus/mprans/cRANS3PSed.cpp
Outdated
| .def("calculateResidual", &cppRANS3PSed_base::calculateResidual) | ||
| .def("calculateJacobian", &cppRANS3PSed_base::calculateJacobian) | ||
| .def("calculateVelocityAverage", &cppRANS3PSed_base::calculateVelocityAverage); | ||
|
|
There was a problem hiding this comment.
This file looks like a duplicata of RANS3PSed.cpp or am I missing something? If this is a duplicate, this defines the module twice. Also I would be in favor of keeping the name of the implementation file consistent with that of the header file. The 'c' prefix makes me think of a file generated by cython while it is not.
proteus/mprans/cRANS3PSed2D.cpp
Outdated
| .def("calculateResidual", &cppRANS3PSed2D_base::calculateResidual) | ||
| .def("calculateJacobian", &cppRANS3PSed2D_base::calculateJacobian) | ||
| .def("calculateVelocityAverage", &cppRANS3PSed2D_base::calculateVelocityAverage); | ||
| } |
There was a problem hiding this comment.
Same comment w.r.t. RANS3PSed2D.cpp
| extra_compile_args=PROTEUS_OPT+['-std=c++14'], | ||
| language='c++'), | ||
| Extension( | ||
| 'mprans.cRANS3PSed2D', |
There was a problem hiding this comment.
Ah you replaced RANS3PSed.cpp with cRANS3PSed.cpp here (same for the 2D file) so the modules are not defined twice. Therefore, the RANS3PSed.cpp and RANS3PSed2D.cpp are not used anymore and could be deleted. However I would be in favor of doing the contrary to keep naming consistency with other modules.
|
Regarding the To summarize, the Now, when you include a header from This way, extension authors only have to be careful when they write the module that contains the initialization routine. Besides, when |
|
LGTM! |
I merged master back into a testing branch I made recently. This is in response to the discussion #1139 . I don't find that this compiles significantly faster, but the design is simpler in my opinion. In this version the 2D and 3D extension modules don't depend on one another, so when you're developing on say, RANS3P2D, it doesn't force recompilation of RANS3P.
Mandatory Checklist
Please ensure that the following criteria are met:
As a general rule of thumb, try to follow PEP8 guidelines.
Description