Proposing a change in interface for CompositionalViscosityPrefactors<dim>::compute_viscosity

[lhy11009]

@danieldouglas92

I’m trying to modify the behavior of the viscosity module by hard-coding certain variables (e.g., temperature). In the calculate_isostrain_viscosities function, the temperature is first processed into the variable temperature_for_viscosity. However, the current interface of
CompositionalViscosityPrefactors::compute_viscosity takes MaterialInputs as an argument and re-parses the temperature from there, rather than using the already processed value.

This makes the workflow less transparent compared to other rheology modules (e.g., diffusion creep), which directly take temperature and pressure values passed down from calculate_isostrain_viscosities. For consistency and clarity, it seems preferable for this interface to follow the same pattern.

Because of this, I think adjusting the interface of compute_viscosity to accept temperature (and possibly pressure) explicitly, rather than re-reading them from MaterialInputs, would be a better design.

If you also agree, I can create a PR.

[danieldouglas92]

Hey @lhy11009

I don't see any issue with this! If I remember the decision to do it this way was because MaterialModel Inputs was needed for other calculations within CompositionalViscosityPrefactors. But I agree that since all the other compute_visscosity functions take in the pressure and temperature that it would be more consistent with the rest of the code and improve readability. If you open a PR I'll be sure to review it ASAP

[lhy11009]

Sure — it doesn’t necessarily need to be merged, but I think it’s worth discussing. I’ll go ahead and wrap up what I’ve done on my branch and open a PR so we can look at it more concretely. (ChatGPT is doing most of the heavy lifting these days 😄.)

I also had another observation about the current implementation. In calculate_isostrain_viscosities, we have this block:

switch (viscosity_prefactor_scheme)
{
case none:
{
factored_viscosities = base_viscosity;
break;
}
}

This feels a bit unusual to me. In most of the other rheology modules, the equivalent logic is handled more explicitly at the higher level within calculate_isostrain_viscosities, rather than being embedded inside the prefactor handling itself.

Again, the implementation works perfectly fine — it’s more a question of readability and consistency to follow the logic of the upper-level calculate_isostrain_viscosities function.

[lhy11009]

I am handlin this in PR #6849

[gassmoeller]

Ah I didnt see this discussion (if you link to a PR with a hashtag in front, it appears in the other discussion as a mention, which is useful to find linked discussions).

Could you still reply to my questions in the PR? Then we have the discussion in one place with the proposed changes.