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

changed default alkalinity to prognostic in main setup only #136

Merged
merged 10 commits into from
Jul 24, 2024

Conversation

glessin
Copy link
Member

@glessin glessin commented Jun 19, 2024

Description of Work

Fixes #119

Testing Instructions

  1. Test whether it works
  2. Correct if it does not
  3. Accept or don't

@glessin glessin closed this Jun 19, 2024
@glessin glessin reopened this Jun 19, 2024
@wathen
Copy link
Member

wathen commented Jul 16, 2024

@glessin, shall I fix the expected values in the tests so they pass? They look relatively similar for a quick browse through the ones that fail

@wathen wathen force-pushed the 119-prognostic_alkalinity branch from 37f51ab to 5a45813 Compare July 24, 2024 10:05
@wathen
Copy link
Member

wathen commented Jul 24, 2024

@glessin I have updated the expected results now so ready for review, can you let the reviewers know it's now ready?

Copy link
Collaborator

@yutiPML yutiPML left a comment

Choose a reason for hiding this comment

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

changes are good, tested and the model compile and runs.
Results show a (negative) trend in TA (to be investigated) but this is not related to the change, hence approved

@yutiPML yutiPML merged commit 6a80806 into master Jul 24, 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.

Prognostic alkalinity as default option
4 participants