Skip to content

Compositional viscosity prefactors interface#6849

Open
lhy11009 wants to merge 2 commits intogeodynamics:mainfrom
lhy11009:compositional_viscosity_prefactors_interface
Open

Compositional viscosity prefactors interface#6849
lhy11009 wants to merge 2 commits intogeodynamics:mainfrom
lhy11009:compositional_viscosity_prefactors_interface

Conversation

@lhy11009
Copy link
Contributor

@lhy11009 lhy11009 commented Feb 3, 2026

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

I change the interface of the CompositionalViscosityPrefactors to be more consistent with the calculate_isostrain_viscosities function.

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

  • I have tested my new feature locally to ensure it is correct.
  • I have created a testcase for the new feature/benchmark in the tests/ directory.
  • I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

@lhy11009
Copy link
Contributor Author

lhy11009 commented Feb 3, 2026

@danieldouglas92 Please take a look at this PR.

This update makes several adjustments to better align the new rheology implementation with the structure used in other material model modules:

I modified the compute_viscosity function so that it now explicitly takes temperature, pressure, fluid_fraction, etc., as input arguments. This brings the interface in line with the approach used by the other rheology modules.

I reordered where this function is called within calculate_isostrain_viscosities. With this change, it is now clearer that the previously computed diffusion and dislocation viscosities are treated as the base viscosity for this module.

Water fugacity is currently first introduced as a variable in calculate_isostrain_viscosities. Following this adjust, we can now also allow users to pass in a constant water fugacity value directly in visco_plastic.cc. This is likely to be broadly useful, and I can implement that extension in a follow-up PR.

@mibillen
Daniel previously implemented this excellent rheology module that includes water fugacity, which is something we have wanted for a long time. This PR only makes a small adjustment to the interface, and one more such PR might follow.

@lhy11009
Copy link
Contributor Author

lhy11009 commented Feb 3, 2026

The commit catalog seems to be messed up a little for a reason I don't know (I just rebased on the main branch), but it seems to contain this previous commit already in the main branch.

@lhy11009 lhy11009 force-pushed the compositional_viscosity_prefactors_interface branch from 09f6232 to f3dc38f Compare February 4, 2026 22:48
@lhy11009
Copy link
Contributor Author

lhy11009 commented Feb 4, 2026

Yes, I rebased, and now the commit list is right.

Copy link
Contributor

@danieldouglas92 danieldouglas92 left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up this code Haoyuan, I think that this looks great! Only one minor comment to remove todo line.

@lhy11009 lhy11009 force-pushed the compositional_viscosity_prefactors_interface branch from f3dc38f to 6cb5df4 Compare February 6, 2026 06:08
@lhy11009
Copy link
Contributor Author

lhy11009 commented Feb 6, 2026

Yes, removed

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

I like the code simplification you did in visco_plastic.cc, but I dont quite understand your change to the interface of compositional_viscosity_prefactors. We usually try to avoid interface changes. Can you summarize what you are trying to achieve? Maybe we can find a way that does not break compatibility with the old code.

Comment on lines 61 to 62
Copy link
Member

Choose a reason for hiding this comment

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

Not your fault, but can you change this to a proper doxygen comment:
/**

  • The flow law ...
  • ...
    */

Comment on lines 44 to 49
template <int dim>
bool
CompositionalViscosityPrefactors<dim>::needs_scaling() const
{
return (viscosity_prefactor_scheme != none);
}
Copy link
Member

Choose a reason for hiding this comment

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

I dont really like the existence of this function, because it exposes information from inside the plugin to the outside world. The same functionality could be put to the beginning of the compute_viscosity() function and then you just return the base_viscosity without multiplying anything. Is there another reason this information is needed outside of this plugin?

@lhy11009 lhy11009 force-pushed the compositional_viscosity_prefactors_interface branch from ffb71e1 to c03f885 Compare February 7, 2026 05:43
@lhy11009 lhy11009 force-pushed the compositional_viscosity_prefactors_interface branch from c03f885 to b518d10 Compare February 7, 2026 05:47
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what happens to this file, my CMakeLists.txt in this branch is the same as origin/main

@gassmoeller
Copy link
Member

This is better, but I still dont like the change to the CompositionalViscosityPrefactors interface. This plugin was intended to offer several different prefactors to viscosity based on values of the compositional fields. It so happens that the fugacity multiplication is the only one that is currently implemented. However, your modified interface limits the plugin to exactly this one case, because it adds the fluid_fraction as additional parameter.
Can you explain more what you intend to do with the bound_fluid_fraction in the future? If you want to be able to set a constant value instead of using the compositional field value, this could be an input parameter of CompositionalViscosityPrefactors and would not require changes outside of the class. I would prefer to contain the change inside CompositionalViscosityPrefactors if possible.

@lhy11009
Copy link
Contributor Author

lhy11009 commented Feb 11, 2026

Okay, I see your point.

My main goals with this change are:

(1) To make temperature and pressure explicit inputs to the interface.

(2) To allow bound_fluid_fraction to be defined as a constant.

I agree that point (2) can be handled without modifying the interface.

However, for point (1), in all other rheology modules temperature and pressure are passed from the upper-level calculate_..._viscosity functions. That leads to two broader questions that I think are worth considering:

a. Interface consistency with other rheology modules
Currently this module behaves differently from the others. Aligning the interface would make the overall structure more uniform and easier to maintain.

b. Flexibility of passing P and T from the top level
Passing pressure and temperature from the top level could be useful in future extensions. For example, a user might want to use adiabatic pressure instead of full dynamic pressure. If that option were implemented, it would ideally be handled consistently across all rheology modules, which is easier if P and T are passed explicitly.

Given these considerations, I see two reasonable paths forward:

  1. For this module, accept temperature and pressure from the upper level, but still keep MaterialInputs as an argument so that additional variables can be queried when needed.

  2. Alternatively, we could consider a more general refactoring of the interfaces of other modules. Instead of passing P and T directly, they could take MaterialInputs as input. Then, at the upper level, we could make a modified copy of MaterialInputs (for example replacing pressure with adiabatic pressure) and pass it consistently to all rheology modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants