-
Notifications
You must be signed in to change notification settings - Fork 97
refactor: constitutive cleanup #3776
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
Conversation
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>
There was a problem hiding this 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
allocateConstitutiveDatamethod calls to useresizeFieldspattern 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.
src/coreComponents/constitutive/solid/ElasticIsotropicPressureDependent.hpp
Show resolved
Hide resolved
src/coreComponents/constitutive/solid/ElasticIsotropicPressureDependent.hpp
Show resolved
Hide resolved
src/coreComponents/constitutive/solid/ElasticIsotropicPressureDependent.hpp
Show resolved
Hide resolved
src/coreComponents/constitutive/solid/ElasticIsotropicPressureDependent.hpp
Show resolved
Hide resolved
| MultiFluidBase::allocateConstitutiveData( parent, numPts ); | ||
|
|
||
| // Zero k-Values to force re-initialisation | ||
| m_kValues.zero(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
dkachuma
left a comment
There was a problem hiding this 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.
src/coreComponents/constitutive/capillaryPressure/CapillaryPressureBase.hpp
Outdated
Show resolved
Hide resolved
| setLabels(); | ||
| } | ||
|
|
||
| void MultiFluidBase::initializeState() const |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
Added a method to initialize the model state.
allocateConstitutiveDatalogicregisterFieldcalls similar to how they look like in solvers (avoid "{}")will fix added TODOs in a follow-up PR