Skip to content

Conversation

@paveltomin
Copy link
Collaborator

@paveltomin paveltomin commented Aug 14, 2025

  • unify allocateConstitutiveData logic
  • remove redundant code (mostly empty destructors, inspired by feat: Linear algebra updates #2427 (comment))
  • make registerField calls similar to how they look like in solvers (avoid "{}")
  • remove duplicative initializers from constructor list

will fix added TODOs in a follow-up PR

@paveltomin paveltomin self-assigned this Aug 14, 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 Aug 14, 2025
@paveltomin paveltomin requested a review from Copilot August 15, 2025 20:34

This comment was marked as outdated.

paveltomin and others added 6 commits August 15, 2025 15:42
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…Dependent.hpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…Dependent.hpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…Dependent.hpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@paveltomin paveltomin requested a review from Copilot August 15, 2025 20:47
Copy link

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 performs a comprehensive constitutive system cleanup, refactoring the allocateConstitutiveData pattern to a more consistent resizeFields approach and removing unnecessary virtual destructors and member variables. The changes standardize memory allocation patterns across all constitutive models and fix spelling errors.

  • Refactors allocateConstitutiveData method calls to use resizeFields pattern consistently
  • Removes virtual destructors and unused member variables across constitutive models
  • Fixes spelling errors in filenames and code (e.g., "Anisotroipic" to "Anisotropic")

Reviewed Changes

Copilot reviewed 149 out of 149 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/coreComponents/physicsSolvers/fluidFlow/SinglePhaseBase.cpp Updates fluid state initialization to use saveConvergedState()
src/coreComponents/physicsSolvers/fluidFlow/FlowSolverBase.cpp Removes unused hydraulic aperture header include
src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseBase.cpp Updates fluid state initialization to use saveConvergedState()
src/coreComponents/constitutive/solid/SolidModelDiscretizationOpsFullyAnisotropic.hpp Fixes spelling error in filename and struct name
src/coreComponents/constitutive/ConstitutiveBase.hpp Adds base resizeFields virtual method
src/coreComponents/constitutive/ConstitutiveBase.cpp Integrates resizeFields into allocateConstitutiveData
Multiple constitutive model files Refactors allocateConstitutiveData to resizeFields and removes virtual destructors

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

MultiFluidBase::allocateConstitutiveData( parent, numPts );

// Zero k-Values to force re-initialisation
m_kValues.zero();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkachuma could you please double-check i didn't messed up your recent fix here

Copy link
Contributor

Choose a reason for hiding this comment

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

We still need the initializeState method.

Copy link
Contributor

@dkachuma dkachuma left a comment

Choose a reason for hiding this comment

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

I think we still need to distinguish initializeState from saveConvergedState. Although initializeState simply calls saveConvergedState for now, we will have instances where there are truly actions that need be done only once.

setLabels();
}

void MultiFluidBase::initializeState() const
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still leave this method. I can see this being useful for some fluid models (in the future). It does give the feel that it will be called once therefore is good for properties that need initialising.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

restored


registerField( fields::cappres::jFuncMultiplier{}, &m_jFuncMultiplier );

registerWrapper( viewKeyStruct::jFunctionWrappersString(), &m_jFuncKernelWrappers ).
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need this for the deliverClone to work properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't break anything after I removed it, but I can revert since I am not exactly sure what is going on here

MultiFluidBase::allocateConstitutiveData( parent, numPts );

// Zero k-Values to force re-initialisation
m_kValues.zero();
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need the initializeState method.

@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Sep 12, 2025
@paveltomin paveltomin added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Sep 13, 2025
@paveltomin paveltomin merged commit 9a12dc4 into develop Sep 14, 2025
25 of 26 checks passed
@paveltomin paveltomin deleted the pt/constitutive-cleanup branch September 14, 2025 20:23
@paveltomin paveltomin restored the pt/constitutive-cleanup branch September 15, 2025 21:37
@paveltomin paveltomin deleted the pt/constitutive-cleanup branch September 15, 2025 21:39
@paveltomin paveltomin restored the pt/constitutive-cleanup branch September 15, 2025 21:41
@paveltomin paveltomin deleted the pt/constitutive-cleanup branch September 15, 2025 21:41
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: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants