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

Feature/mfmehari/krok obio #871

Merged
merged 18 commits into from
Feb 21, 2024
Merged

Feature/mfmehari/krok obio #871

merged 18 commits into from
Feb 21, 2024

Conversation

mfmehari
Copy link
Contributor

OBIO (NOBM) is integrated into the Krok Model.
It is still in testing phase.
It is a zero diff PR.

@mfmehari mfmehari added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Dec 13, 2023
@mfmehari mfmehari requested review from a team as code owners December 13, 2023 16:44
@sanAkel
Copy link
Contributor

sanAkel commented Dec 13, 2023

Thanks @mfmehari I'll take a look at this tomorrow (late in the day).

Can you list all the ways you tested this PR. Example coupled ( which ocean, what resolution(s)), also you must do an uncoupled model run - it should preserve answers in both (uncoupled and coupled) modes- this is needed because you have changed source code at a lot of places.

tclune
tclune previously approved these changes Dec 13, 2023
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

cmake changes ok

@mfmehari
Copy link
Contributor Author

mfmehari commented Dec 13, 2023

Thanks @mfmehari I'll take a look at this tomorrow (late in the day).

Can you list all the ways you tested this PR. Example coupled ( which ocean, what resolution(s)), also you must do an uncoupled model run - it should preserve answers in both (uncoupled and coupled) modes- this is needed because you have changed source code at a lot of places.

@sanAkel
It is tested with the coupled model run:
AGCM: C12 (IM:12 x JM:72) and LM:72
OGCM (MOM6): IM:72 x JM:36 and LM:50
Okay, I will do the uncoupled run.
Thank you!

@mfmehari
Copy link
Contributor Author

Thanks @mfmehari I'll take a look at this tomorrow (late in the day).
Can you list all the ways you tested this PR. Example coupled ( which ocean, what resolution(s)), also you must do an uncoupled model run - it should preserve answers in both (uncoupled and coupled) modes- this is needed because you have changed source code at a lot of places.

@sanAkel It is tested with the coupled model run: AGCM: C12 (IM:12 x JM:72) and LM:72 OGCM (MOM6): IM:72 x JM:36 and LM:50 Okay, I will do the uncoupled run. Thank you!

@mfmehari mfmehari closed this Dec 14, 2023
@mfmehari
Copy link
Contributor Author

Thanks @mfmehari I'll take a look at this tomorrow (late in the day).
Can you list all the ways you tested this PR. Example coupled ( which ocean, what resolution(s)), also you must do an uncoupled model run - it should preserve answers in both (uncoupled and coupled) modes- this is needed because you have changed source code at a lot of places.

@sanAkel It is tested with the coupled model run: AGCM: C12 (IM:12 x JM:72) and LM:72 OGCM (MOM6): IM:72 x JM:36 and LM:50 Okay, I will do the uncoupled run. Thank you!

@sanAkel Hi Santha,
Tested the uncoupled run and it has a zero diff.
AGCM: C12 (im:12 x JM:72) and LM:72
Data_Ocean Horizontal Resolution code: o1 (1 -deg, 360x180 Reynolds)
Thank you!

@mfmehari mfmehari reopened this Dec 14, 2023
@sanAkel sanAkel added the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Dec 20, 2023
@sanAkel sanAkel removed their assignment Dec 20, 2023
@sanAkel sanAkel self-requested a review December 20, 2023 12:44
@sanAkel sanAkel removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Dec 20, 2023
@mfmehari mfmehari dismissed stale reviews from sdrabenh and bena-nasa via 9259e51 February 7, 2024 20:05
sdrabenh
sdrabenh previously approved these changes Feb 7, 2024
gmao-rreichle
gmao-rreichle previously approved these changes Feb 7, 2024
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

Changes ok from Land perspective. Would appreciate a more descriptive PR title.

GEOS_GcmGridComp.F90 Outdated Show resolved Hide resolved
Copy link
Contributor

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

@mfmehari Please consider making suggested changes.

lcandre2
lcandre2 previously approved these changes Feb 8, 2024
@mathomp4
Copy link
Member

mathomp4 commented Feb 8, 2024

@mfmehari Please consider making suggested changes.

@sanAkel I guess the only issue is that the rest of the DO_ resources in the gridcomp around there do not use logicals.

    call MAPL_GetResource ( MAPL, DO_OBIO, Label="USE_OCEANOBIOGEOCHEM:", DEFAULT=0, _RC)
    call MAPL_GetResource ( MAPL, DO_DATASEA, Label="USE_DATASEA:", DEFAULT=1, _RC)
    call MAPL_GetResource ( MAPL, DO_WAVES, Label="USE_WAVES:", DEFAULT=0, _RC)
    call MAPL_GetResource ( MAPL, DO_SEA_SPRAY, Label="USE_SEA_SPRAY:", DEFAULT=0, _RC)

I mean, it's what they should do but... I guess the question is should we be consistent?

@sanAkel
Copy link
Contributor

sanAkel commented Feb 8, 2024

@mfmehari Please consider making suggested changes.

@sanAkel I guess the only issue is that the rest of the DO_ resources in the gridcomp around there do not use logicals.

    call MAPL_GetResource ( MAPL, DO_OBIO, Label="USE_OCEANOBIOGEOCHEM:", DEFAULT=0, _RC)
    call MAPL_GetResource ( MAPL, DO_DATASEA, Label="USE_DATASEA:", DEFAULT=1, _RC)
    call MAPL_GetResource ( MAPL, DO_WAVES, Label="USE_WAVES:", DEFAULT=0, _RC)
    call MAPL_GetResource ( MAPL, DO_SEA_SPRAY, Label="USE_SEA_SPRAY:", DEFAULT=0, _RC)

I mean, it's what they should do but... I guess the question is should we be consistent?

@mathomp4 (and @mfmehari), your ⬆️ question is a relevant one. Following are my current thoughts.

  1. This PR is to a repo that has decades of (...) built in. There is absolutely no way we can accomplish should we be consistent? unless one of us spends some time to clean this (logical) aspect and indeed so many, many, ... others; ultimately we'll be looking at a rewrite! (Just look at surface GC .F90, look at its length and scope? Anyway let's leave it out here).
  2. My suggestions to @mfmehari are to the point of what they _should_ do so things are better going forward. I had the same comments earlier for the waves_PR from @adarmenov and others before it; I'm being consistent in my suggestions.

@mathomp4 did I answer your question(s)?

@mfmehari
Copy link
Contributor Author

mfmehari commented Feb 8, 2024

@sanAkel If we are okay with DO_DATA_ATM4OCN only being in logical form, I could go ahead change its type, thank you!

@sanAkel
Copy link
Contributor

sanAkel commented Feb 8, 2024

@sanAkel If we are okay with DO_DATA_ATM4OCN only being in logical form, I could go ahead change its type, thank you!

👍 Let's just try to do good here on ...

@mfmehari mfmehari dismissed stale reviews from lcandre2, gmao-rreichle, and sdrabenh via f9819aa February 8, 2024 18:44
@mfmehari
Copy link
Contributor Author

mfmehari commented Feb 8, 2024

@sanAkel The type of DO_DATA_ATM4OCN has been changed from int to logical.

@mathomp4 mathomp4 requested a review from a team February 21, 2024 13:57
@sdrabenh sdrabenh merged commit 6abec00 into develop Feb 21, 2024
9 checks passed
@sdrabenh sdrabenh deleted the feature/mfmehari/Krok_Obio branch February 21, 2024 14:01
@mfmehari
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants