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 namelist creating for multiple instances #406

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

blcc
Copy link
Collaborator

@blcc blcc commented Sep 26, 2024

Get correct NTASK_OCN in buildcpp, which will be NINST_OCN times when run in multiple instance.
Move pg_blom into instance loop to reset it in every instance.

Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

@blcc - I think this looks fine, but suggest not to overwrite ntasks_ocn.
Can merge this one you have made the change (or explain why you want to keep the code as it is).

Comment on lines 39 to 42
ntasks_ocn = case.get_value("NTASKS_OCN")
objroot = case.get_value("OBJROOT")
if case.get_value("NINST_OCN") > 1:
ntasks_ocn = case.get_value("NTASKS_PER_INST_OCN")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to define ntasks_ocn once, not overwrite

if (case.get_value("NINST_OCN") > 1) then
   ntasks_ocn = case.get_value("NTASKS_PER_INST_OCN")
else
   ntasks_ocn = case.get_value("NTASKS_OCN")
end if

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, it is more clear with if-else.
I'll send updated PR tomorrow.

@TomasTorsvik TomasTorsvik self-assigned this Sep 26, 2024
@TomasTorsvik TomasTorsvik added the enhancement New feature or request label Sep 26, 2024
@blcc
Copy link
Collaborator Author

blcc commented Sep 27, 2024

Hi Tomas,
I just found NTASKS_PER_INST_OCN output same result with NTASKS_OCN when only one instance.
So that I replace it.

@TomasTorsvik
Copy link
Contributor

TomasTorsvik commented Sep 27, 2024

@blcc - I'm not convinced this is a good solution, since users may expect that setting the "NTASKS_OCN" option will set the ntasks_ocn variable. We should not change functionality, especially in the CMIP6 version of the model. I think its OK to make ntasks_ocn dependant on "NINST_OCN", since I don't think anyone will change "NINST_OCN" without knowing what they are doing.

@blcc
Copy link
Collaborator Author

blcc commented Sep 27, 2024

It also make point.
I change it back.

@TomasTorsvik TomasTorsvik merged commit b53a5da into NorESMhub:release-1.4 Sep 27, 2024
6 checks passed
@TomasTorsvik
Copy link
Contributor

@blcc - I merged this PR, and have created a new tag v1.4.3 on the release-1.4 branch, for reference when you update the Externals.cfg file.

@blcc blcc deleted the release-1.4 branch September 27, 2024 09:15
@blcc
Copy link
Collaborator Author

blcc commented Sep 27, 2024

Thank you very much.

@blcc blcc restored the release-1.4 branch September 30, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants