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

Fix bugs for custom diffusions #429

Merged

Conversation

illorenzo7
Copy link
Contributor

@illorenzo7 illorenzo7 commented Dec 25, 2022

There was an issue in the logic for specifying custom scalar diffusions. Only nu_type, kappa_type, and eta_type were checked to be 3, with the result that if custom scalar diffusions were used, they might not be read from the binary file (unless reference_type was 4, in which case, the custom reference file had already been read).

Similarly, for reference_type != 4 there was an indexing issue in temp_constants, that led to seg faults if one of the reference_types was 3. @feathern perhaps this is related to the issues you were seeing regarding custom reference and the scalar fields?

Anyway, this pull request should fix these bugs. It incorporates my other pull requests (#427 and #426).

Finally, there is an outstanding issue (that is supported by my preliminary testing) with custom diffusions (and again reference_type !=4 ). If one custom diffusion is used (but not all), then temp_functions for the other diffusions will be whatever ra_functions used to be before the initialize_diffusivity calls (likely zero). I think the actual diffusions in Rayleigh will be set correctly (and so will ra_functions), but then ra_functions will get overwritten by the incorrect temp_functions. The code will then run correctly but the output of the equation_coefficients file will be corrupt. Leaving this for now.

@illorenzo7
Copy link
Contributor Author

illorenzo7 commented Dec 31, 2022

Update: there was another bug in initializing ref%chi_buoyancy_coeff (the first index was zero instead of one). Someone more familiar with the scalar fields (@feathern or @cianwilson) should check on this to make sure everything is going as intended as far as the indexing is concerned.

My (potentially flawed) interpretation: Say j = 1, 2, 3, ... , n_active_scalars. From what I understand, the user can set the constant multiplying the relevant function via the [11 + (j-1)*2]th constant. They can set the constant multiplying the buoyancy force (f_2) via the [12+(j-1)*2]th constant. This is at least what the code seems to reflect.

It also seems that the buoyancy scalar diffusion constants are never set for reference_types = 1, 2, 3. Since I'm already thinking about this today, I will fix this in a separate pull request.

And happy new year! I know I am working at odd times...I took my vacation two weeks ago ;-p

@illorenzo7
Copy link
Contributor Author

Great! The same fix for #427 works here. I'll do the others now.

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.

This looks good save for two things.

  1. The conflict ('object' vs. 'derived type') that needs to be resolved.
  2. As we discussed offline, please remove the comment about a bug that needs to be fixed (maybe just paste that into a brief issue note in Github).

@illorenzo7 illorenzo7 force-pushed the fix_logic_custom_scalar_diffusion branch from a327177 to 1c0fbbd Compare January 6, 2023 00:02
@feathern
Copy link
Contributor

feathern commented Jan 6, 2023

Great -- merging.

@feathern feathern merged commit e4cf17f into geodynamics:main Jan 6, 2023
@illorenzo7 illorenzo7 deleted the fix_logic_custom_scalar_diffusion branch February 14, 2023 20:04
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