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

Split thermodynamic_coefficients_type #172

Merged
merged 4 commits into from
Jun 19, 2017
Merged

Split thermodynamic_coefficients_type #172

merged 4 commits into from
Jun 19, 2017

Conversation

mnlevy1981
Copy link
Collaborator

Per #19, have put dic, ta, pt, and sit in their own data type.

I used the name thermodynamic_species_concentration_type based on the comment above the declaration (declarations for function coefficients & species concentrations) but am happy to change it to something more appropriate.

Verified this code builds on the usual machines / compilers, and that it does not change answers in my one-off ciso gx3v7 test on hobart (nag) or cheyenne (intel).

Per #19, have put dic, ta, pt, and sit in their own data type.
@mnlevy1981
Copy link
Collaborator Author

For this PR, I was uncertain about what to name the new derived type -- does the thermodynamic prefix in thermodynamic_species_concentration_type make sense? I'm happy to change it, and intentionally didn't change the whitespace in variable declaration blocks in case the species_concentration line shrinks.

@klindsay28
Copy link
Collaborator

Given that the name of the module contains co2calc, I suggest renaming
thermodynamic_coefficients_type to co2calc_coeffs_type
thermodynamic_coefficients_type to co2calc_state_type

I see an oppurtunity for the related renamings to make names more consistent
co3_coeffs to co2calc_coeffs
marbl_comp_co3_coeffs to comp_co2calc_coeffs (also list this as private at the module level)

thermodynamic_coefficients_type          -> co2calc_coeffs_type
thermodynamic_species_concentration_type -> co2calc_state_type

Next commit will have more of the recommended cleanup:
* co3_coeffs -> co2calc_coeffs
* better subroutine names in module
Per the second suggestion of the code review, co3_coeffs has been
renamed co2calc_coeffs (and species_concentration has been renamed
co2calc_state to match the derived type name). marbl_comp_co3_coeffs has
been renamed comp_co2calc_coeffs, and explicitly declared as private in
the module. And I believe white space in variable declarations / code
continuation lines is all lined up correctly.
@mnlevy1981
Copy link
Collaborator Author

@klindsay28 -- can you take a look at this PR again when you have a chance? I believe I've addressed everything in your comment. Thanks!

Copy link
Collaborator

@klindsay28 klindsay28 left a comment

Choose a reason for hiding this comment

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

looks good

@mnlevy1981 mnlevy1981 merged commit 8f49a2b into marbl-ecosys:master Jun 19, 2017
@mnlevy1981 mnlevy1981 deleted the enhancement/cleanup_thermo_coeff_type branch June 19, 2017 22:11
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