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

Remove unused params in oxygen module for iswO2 #135

Merged
merged 11 commits into from
Aug 23, 2024
Merged

Conversation

wathen
Copy link
Member

@wathen wathen commented Jun 6, 2024

Description of Work

Fixes #51

Testing Instructions

  1. Check right optional parameters have been removed

@wathen wathen requested a review from jimc101 June 6, 2024 10:19
@wathen
Copy link
Member Author

wathen commented Jun 6, 2024

@jimc101 from the description in the issue you say that "legacy ERSEM" and "Weiss 1970" are the only options to choice from though it looks like the options are "legacy ERSEM" and "Wanninkhof 2014" from the code

ersem/src/oxygen.F90

Lines 101 to 109 in 7acae16

if (legacy_ersem_compatibility) then
! Old formulation for the Schmidt number,
! left for documentation and back compatibility
sc = max(0.0_rk, 1953.4_rk - 128._rk*ETW + 3.9918_rk*ETW**2 - 0.050091_rk*ETW**3)
else
! New formulation for the Schmidt number for O2 following Wanninkhof 2014
T = max(min(ETW,40.0_rk), -2.0_rk)
sc = 1920.4_rk - 135.6_rk*T + 5.2122_rk*T**2._rk - 0.10939_rk*T**3 + 0.00093777_rk*T**4._rk
endif

Copy link
Member

@jimc101 jimc101 left a comment

Choose a reason for hiding this comment

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

Sorry, this isn't right. The option relates to the code here and the way the saturation concentration is calculated.

This option should be reduced to legacy (or old) ERSEM and Weiss 1970. I would put Weiss first, since this is the option most users are likely to select. Also, there is no guard for invalid values. So something like:

if weiss:
    ...
elif old_ersem:
    ...
else
    throw exception
fi

could work. These would need to be matched up in the call to get_parameter. What do you think?

@wathen
Copy link
Member Author

wathen commented Aug 1, 2024

Ahh I see, is get_parameter a FABM thing?

@wathen
Copy link
Member Author

wathen commented Aug 1, 2024

@jimc101 having a safe guide for an incorrect parameter would be good in most places in the code though that would be done in a separate issue

@jimc101
Copy link
Member

jimc101 commented Aug 2, 2024

Sorry - I recommend you switch to just deleting the invalid options in the text string and not reordering the loop. This can be addressed in a separate issue (under optimisations). As you say, all valid options should also be tested, but again this can be addressed through a separate issue.

Copy link
Member

@jimc101 jimc101 left a comment

Choose a reason for hiding this comment

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

Okay - looks good.

@wathen wathen merged commit a2429e2 into master Aug 23, 2024
5 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.

The description of the oxygen parameter iswO2 is incorrect
2 participants