Xiangyu-Hu / SPHinXsys

SPHinXsys provides C++ APIs for engineering simulation and optimization. It aims at complex systems driven by fluid, structure, multi-body dynamics and beyond. The multi-physics library is based on a unique and unified computational framework by which strong coupling has been achieved for all involved physics.
https://www.sphinxsys.org/
Apache License 2.0
259 stars 199 forks source link

Merge eigen with master #172

Closed DrChiZhang closed 1 year ago

DrChiZhang commented 1 year ago

The main modifications is to implement the eigen, and other modifications. 1, Replace vector and matrix from SimTK with that from Eigen. 2, Split the function of SPHAdaptation::computeReferenceNumberDensity to 2D annd 3D folder to avoid "ambiguous error". 3, Move the OrthotropicSolid to 3D folder as it's generally 3D implementation. 4, Add new DFG funding NO. 5, Remove the eulerian test from ctest. 6, Remove author information form .cpp file, only kepp in .h file. Remove version information in .h files. 7, Replace ObserveBody with ProbeBody.

Xiangyu-Hu commented 1 year ago

The main modifications is to implement the eigen, and other modifications. 1, Replace vector and matrix from SimTK with that from Eigen. 2, Split the function of SPHAdaptation::computeReferenceNumberDensity to 2D annd 3D folder to avoid "ambiguous error". 3, Move the OrthotropicSolid to 3D folder as it's generally 3D implementation. 4, Add new DFG funding NO. 5, Remove the eulerian test from ctest. 6, Remove author information form .cpp file, only kepp in .h file. Remove version information in .h files. 7, Replace ObserveBody with ProbeBody.

Point 2, I do not think that there is ambiguous as the functions have different type of input parameters. Ambiguous only happens when the function signatures are exact the same.

DrChiZhang commented 1 year ago

When Matrix::Zero() is used as a default input parameter, ambiguity of overloaded function takes place due to the fact that Matrix::Zero() is not a matrix, but an Eigen expression.

DrChiZhang commented 1 year ago

For example: void Foo(Vec2d v) {...} void Foo(Vec3d v) {...} int main{ Foo(Vecd::Zero); return 0; } This may leads to ambiguous error if we put 2D and 3D implementation in the same source file.

FabienPean-Virtonomy commented 1 year ago

The alternative would be to evaluate the expression at the call site directly.

#include<Eigen/Eigen>
using Vec2d = Eigen::Vector2<double>;
using Vec3d = Eigen::Vector3<double>;
using Vecd = Vec3d;
void Foo(Vec2d v) {}
void Foo(Vec3d v) {}
int main() {
    Foo(Vecd::Zero().eval());
    return 0;
}
FabienPean-Virtonomy commented 1 year ago

The main modifications is to implement the eigen, and other modifications. 1, Replace vector and matrix from SimTK with that from Eigen. 2, Split the function of SPHAdaptation::computeReferenceNumberDensity to 2D annd 3D folder to avoid "ambiguous error". 3, Move the OrthotropicSolid to 3D folder as it's generally 3D implementation. 4, Add new DFG funding NO. 5, Remove the eulerian test from ctest. 6, Remove author information form .cpp file, only kepp in .h file. Remove version information in .h files. 7, Replace ObserveBody with ProbeBody.

Thanks, it helps to have this list to guide the code review. Though it would be strongly preferable to not mix the changes and make a separate PR containing each one aspect (1-2-5/3/4/6/7).

DrChiZhang commented 1 year ago

The alternative would be to evaluate the expression at the call site directly.

#include<Eigen/Eigen>
using Vec2d = Eigen::Vector2<double>;
using Vec3d = Eigen::Vector3<double>;
using Vecd = Vec3d;
void Foo(Vec2d v) {}
void Foo(Vec3d v) {}
int main() {
    Foo(Vecd::Zero().eval());
    return 0;
}

Sure, I just choose the straightforward solution.

DrChiZhang commented 1 year ago

Sorry for the inconvenience, I will follow your suggested principle next modification.

Xiangyu-Hu commented 1 year ago

Another issue is: if you modify the observer class to probe, then the places relevant to the such change should be updated. For example some places where the word 'probe' is already used should be changed to avoid misunderstanding. Basically, probe is for local value. Therefore, the global values obtaining by summation approaches should not be call probed value.

DrChiZhang commented 1 year ago

Another issue is: if you modify the observer class to probe, then the places relevant to the such change should be updated. For example some places where the word 'probe' is already used should be changed to avoid misunderstanding. Basically, probe is for local value. Therefore, the global values obtaining by summation approaches should not be call probed value.

Ok, I will modify these in the following code refactoring.

DrChiZhang commented 1 year ago

Now the ProbeBody and the adaptation has been reversed.

Xiangyu-Hu commented 1 year ago

I am happy to see active discussions on pull request.

FabienPean-Virtonomy commented 1 year ago

We should be very careful when use un-initialized variables. Lots of bugs come from this issue.

Indeed, I used it in that context because Vec2d/Vec3d is only used for tag dispatch and overload resolution, not in the body of the method. I'm not sure it is the best solution to the problem (your initial choice of splitting might be cleaner in the end). But at this point, better trying to wrap up the PR.

Note that the problem with Orthotropic material is not resolved and will come bite soon again depending on how the library is built (static vs shared). It means either offloading the whole implementation in elastic_solid_supplementary.h but then the material is not found by including all_materials.h without further changes, or building a dummy 2d version to preserve symmetry.

DrChiZhang commented 1 year ago

I can reverse it back to the shared folder, while all vector and matd contained should be change to its 3D form. However, the logic is wrong in this case, as a function in shared folder is only works for 2D modeling.

DrChiZhang commented 1 year ago

Thanks for the effort.

FabienPean-Virtonomy commented 1 year ago

@ChiZhangatTUM I made a fix following the existing convention. Dimension-specific functions are implemented in each version of the library. The 2D version is reporting an error since not fully implemented. The other functions have been made dimension- independent but required to constexpr some variables (shouldn't be a problem as they should be constexpr anyway). If this is ok with you, then on my side it is fine to merge it after CI passes.

DrChiZhang commented 1 year ago

@ChiZhangatTUM I made a fix following the existing convention. Dimension-specific functions are implemented in each version of the library. The 2D version is reporting an error since not fully implemented. The other functions have been made dimension- agnostic but required to constexpr some variables (shouldn't be a problem as they should be constexpr anyway). If this is ok with you, then on my side it is fine to merge it after CI passes.

Thanks for the work. The present form looks good. We can merge it after all the tests pass. Thanks for the cooperation.

DrChiZhang commented 1 year ago

@BenceVirtonomy the quested change is not essential, would you please withdraw it, and then we can merge.

BenceVirtonomy commented 1 year ago

Approved, thank you!