Skip to content
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

Set f_14 depending on convention; don't reset c's and f's for diffusion types = 3 #505

Merged
merged 3 commits into from
May 30, 2024

Conversation

illorenzo7
Copy link
Contributor

This pull request lumps in the setting of f_14 with c_11 to coincide with different conventions depending on reference_type. It also "protects" various c's and f's from being reset when the user specifies a diffusion using a custom reference (and therefore, set the c and f directly).

This change should not alter the functionality of the code at all (and it should not alter the internal "reference" object), but it should change the output of the equation_coefficients file in certain circumstances.

@feathern
Copy link
Contributor

Sorry that it took me a minute to get to this. I think there's a (maybe) potential bug in that the user can selected reference_type=2 and also use 'with_custom_reference" to specify a background dsdr. It seems like the logic added here will unset the function and constant associated with dsdr.

@illorenzo7
Copy link
Contributor Author

illorenzo7 commented May 29, 2024

@feathern Darn, you're right. In fact, I think it was already a bug, because c_11 was already reset to zero in the case you were mentioning.

I think the cleanest fix would be to make a new flag (say "with_custom_dsdr") that gets set to True if the user sets dsdr via the "with custom" framework. Then only go through the whole block setting c_11 and f_14 if that flag is false. (Otherwise, both c_11 and f_14 were already set).

What do you think?

@illorenzo7
Copy link
Contributor Author

@feathern. OK, from some limited testing, I think there were a number of bugs after introducing c_11 (beginning I believe with request #489). The first you already pointed out. The second is that c_11 was never set to 1 if the user didn't set it, remaining zero even after "with_custom_functions = 14" or similar was specified in main_input. Both these bugs should now be fixed. I also added some warning statements if the user modifies reference_types 1 or 2 which physically assume dsdr = 0.

Copy link
Contributor

@feathern feathern left a comment

Choose a reason for hiding this comment

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

Great -- thanks for taking care of the other logical bugs. Approved.

@feathern feathern merged commit 21b0a1d into geodynamics:main May 30, 2024
5 of 7 checks passed
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.

2 participants