Skip to content

Conversation

paveltomin
Copy link
Collaborator

@paveltomin paveltomin commented Sep 1, 2025

implement proposal from #1876, for flow solvers only for now

also move zformulation into a separate namespace and simplify flux compute kernel call logic a bit

@paveltomin paveltomin self-assigned this Sep 1, 2025
@paveltomin paveltomin added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Sep 1, 2025
@paveltomin
Copy link
Collaborator Author

This builds and tests are ok, ready for a look

@paveltomin paveltomin requested a review from Copilot September 3, 2025 23:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the nested namespace structure for flow solver kernels, implementing a cleaner organization pattern with nested namespaces under kernels. The refactoring primarily affects the flow solver components, moving kernel classes into structured namespaces like kernels::fluidFlow::singlePhase, kernels::wells::compositional, etc. Additionally, the zformulation functionality is moved into a separate namespace and some code is simplified.

Key changes include:

  • Reorganization of kernel namespaces from flat structures to nested hierarchies
  • Introduction of using namespace declarations to maintain compatibility
  • Moving zFormulation kernels to dedicated namespace structure
  • Minor code simplifications in flux computation logic

Reviewed Changes

Copilot reviewed 144 out of 144 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Test files (.cpp) Updated namespace usage with new using declarations for compositional and single phase well kernels
Kernel headers (.hpp) Restructured namespace declarations from flat to nested (e.g., singlePhaseWellKernelskernels::wells::singlePhase)
Implementation files (.cpp) Updated namespace declarations to match new nested structure
Physics solver files Simplified flux computation logic and updated namespace references
zFormulation files Moved to dedicated zformulation namespace and renamed some classes/files for consistency
Comments suppressed due to low confidence (4)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

m_useSimpleAccumulation( 1 ),
m_minCompDens( isothermalCompositionalMultiphaseBaseKernels::minDensForDivision ),
m_minCompFrac( isothermalCompositionalMultiphaseBaseKernels::minCompFracForDivision )
m_minCompDens( kernels::fluidFlow::compositional::minDensForDivision ),
Copy link
Contributor

@MelReyCG MelReyCG Sep 5, 2025

Choose a reason for hiding this comment

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

Why not having a kernel namespace per namespace? (when needed)

Suggested change
m_minCompDens( kernels::fluidFlow::compositional::minDensForDivision ),
m_minCompDens( fluidFlow::compositional::kernels::minDensForDivision ),

As it is proposed here, it seems like a duplicated tree of solver namespaces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rrsettgast @MelReyCG current version is like that:
image
i think what @MelReyCG is suggesting makes a lot of sense and i can swap it around, just confirm that this is the choice
i can also try to wrap solvers into namespaces in a follow-up

Copy link
Member

Choose a reason for hiding this comment

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

@paveltomin I would agree with swapping the kernels to the end

Copy link
Contributor

Choose a reason for hiding this comment

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

@rrsettgast @MelReyCG @paveltomin
Thinking about future consolidation or not... In Jacobian assemble singlePhase and compositional specialization implement the same interfaces... separation of single phase and compositional introduces a layer that may not be needed. Ideally there would be one fluid flow and inside its assemble/update/check methods, switches/factories to call the specialized versions based on model definition. Why duplicate the algorithmic flow code when it doesn't provide any performance optimizations. having single algorithmic flow at a higher level with specializations inside the algorithmic component methods might be easier to maintain.. going down that path maybe something like fluildFlow:kernels:: would be more appropriate...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i switched to
fluidFlow::kernels::compositional::...
that seems logical and it matches the file structure, and it allows to share some kernels and definitions defined at top level between single-phase and compositional in a more clean way
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MelReyCG @rrsettgast @tjb-ltk is it good now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not fully involved in this discussion, but I agree that matching the namespace to the file structure is more logical and cleaner.

Pavel Tomin added 2 commits September 15, 2025 17:36
@paveltomin paveltomin added the flag: no rebaseline Does not require rebaseline label Sep 16, 2025
typename FluidType::KernelWrapper fluidWrapper = castedFluid.createKernelWrapper();

thermalCompositionalMultiphaseBaseKernels::
kernels::fluidFlow::compositional::
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a thermal version of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this one is general, both for thermal and isothermal

localMatrix,
localRhs );
}
assembleFluxTerms( dt,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer need the jump stabilised version of this?

I see now where this is moved to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline flag: ready for review type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants