Skip to content
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

Move particle generators to their own folder #3068

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

TotoGaz
Copy link
Contributor

@TotoGaz TotoGaz commented Apr 6, 2024

Remove the dependency (and the transitive dependencies) to SpatialPartition in the mesh/generators "package".

@TotoGaz TotoGaz added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Apr 6, 2024
@TotoGaz TotoGaz self-assigned this Apr 6, 2024
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 71.49758% with 59 lines in your changes are missing coverage. Please review.

Project coverage is 53.23%. Comparing base (9289b76) to head (2564a74).

Files Patch % Lines
...Components/mesh/generators/PartitionDescriptor.cpp 78.26% 20 Missing ⚠️
...h/particleGenerators/ParticleMeshGeneratorBase.cpp 15.78% 16 Missing ⚠️
src/coreComponents/mesh/MeshManager.cpp 59.37% 13 Missing ⚠️
...onents/mesh/mpiCommunications/SpatialPartition.cpp 20.00% 4 Missing ⚠️
...ents/mesh/generators/InternalWellboreGenerator.cpp 0.00% 3 Missing ⚠️
...Components/mesh/generators/PartitionDescriptor.hpp 77.77% 2 Missing ⚠️
.../mesh/particleGenerators/ParticleMeshGenerator.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3068      +/-   ##
===========================================
+ Coverage    53.19%   53.23%   +0.03%     
===========================================
  Files          989      992       +3     
  Lines        83646    83768     +122     
===========================================
+ Hits         44498    44592      +94     
- Misses       39148    39176      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TotoGaz TotoGaz removed the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Apr 8, 2024
@TotoGaz
Copy link
Contributor Author

TotoGaz commented Apr 8, 2024

Hello @cmcrook5 @homel1

I've tried to split the Particle mesh specials from the standard ones.
To do so I've move all the Particle* classes to the particleGenerators folder (plus some other tweaks).
The architecture is kind of clearer also: what's dedicated to particles is clearly located.

Still remains this question of SpatialPartition which embeds some specific elements for particles.
I've tried to separate by adding a PartitionDescriptor (which also duplicates a little bit of code, which can be acceptable as a temporary solution)
It's working for serial mpm simulations, but for parallel, in some cases yes, in other cases no. It's more random.
My guess is that in the MeshManager::generateMeshes we transfer the result from the new PartitionDescriptor to the SpatialPartition, but something's wrong in there.
Other than that, the failing tests (on the CI) are

FAILED:  3 python3(impermeableFault_smoke_01_2_restartcheck), python3(permeableFault_smoke_01_2_restartcheck), python3(mpm_singleParticle_01_2_restartcheck)
TIMEOUT:  3 geosx(pennyShapedToughnessDominated_smoke_01_1_geos), geosx(pennyShapedViscosityDominated_smoke_01_1_geos), geosx(pknViscosityDominated_smoke_01_1_geos)

which tend to indicate that it's not so bad since currently

FAILED:  2 python3(impermeableFault_smoke_01_2_restartcheck), python3(permeableFault_smoke_01_2_restartcheck)
TIMEOUT:  3 geosx(pennyShapedToughnessDominated_smoke_01_1_geos), geosx(pennyShapedViscosityDominated_smoke_01_1_geos), geosx(pknViscosityDominated_smoke_01_1_geos)

are known to fail.

Is there any chance that you can handle this refactoring?
Maybe you can find what needs to be updated. Or if necessary handle some inheritance to deal with your partitioning?
We can discuss it when you're ready.

@cmcrook5
Copy link
Contributor

cmcrook5 commented Apr 9, 2024

@TotoGaz I pulled your branch and I'm trying to take a look at why the single particle test is failing. However, I'm having some trouble building your branch. I get the follolwing error:

In file included from /usr/WS1/crook5/GEOS_PR/src/coreComponents/linearAlgebra/interfaces/hypre/HypreMGR.cpp:34:
/usr/WS1/crook5/GEOS_PR/src/coreComponents/linearAlgebra/interfaces/hypre/mgrStrategies/SinglePhasePoromechanicsEmbeddedFractures.hpp:117:27: error: use of undeclared
      identifier 'HYPRE_MGRSetFSolverAtLevel'
    GEOS_LAI_CHECK_ERROR( HYPRE_MGRSetFSolverAtLevel( 1, precond.ptr, mgrData.mechSolver.ptr ) );
                          ^
In file included from /usr/WS1/crook5/GEOS_PR/src/coreComponents/linearAlgebra/interfaces/hypre/HypreMGR.cpp:35:
/usr/WS1/crook5/GEOS_PR/src/coreComponents/linearAlgebra/interfaces/hypre/mgrStrategies/SinglePhasePoromechanicsConformingFractures.hpp:124:27: error: use of undeclared
      identifier 'HYPRE_MGRSetFSolverAtLevel'
    GEOS_LAI_CHECK_ERROR( HYPRE_MGRSetFSolverAtLevel( 1, precond.ptr, mgrData.mechSolver.ptr ) );
                          ^

@TotoGaz
Copy link
Contributor Author

TotoGaz commented Apr 9, 2024

Hi @cmcrook5 thanks you for your actions!
Are you using the latest version of the TPLs?

@TotoGaz
Copy link
Contributor Author

TotoGaz commented Apr 24, 2024

Hello @cmcrook5 did you manage to compile the branch?

@cmcrook5
Copy link
Contributor

@TotoGaz yes, I was able to after updating the TPLs, thanks.

@TotoGaz TotoGaz added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Apr 30, 2024
@TotoGaz TotoGaz added ci: run CUDA builds Allows to triggers (costly) CUDA jobs and removed ci: run CUDA builds Allows to triggers (costly) CUDA jobs labels May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run integrated tests Allows to run the integrated tests in GEOS CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants