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

Issue459 oasis ocean coupling #641

Draft
wants to merge 33 commits into
base: develop
Choose a base branch
from
Draft

Conversation

einola
Copy link
Member

@einola einola commented Aug 19, 2024

Pull Request Title

Fixes #459

Task List

  • Defined the tests that specify a complete and functioning change (It may help to create a design specification & test specification)
  • Implemented the source code change that satisfies the tests
  • Documented the feature by providing worked example
  • Updated the README or other documentation
  • Completed the pre-Request checklist below

Change Description

Please write a summary of the change and specify which Issue this fixes. This should cover the motivation for making the change and provide almost enough context to review the change in isolation. While the issue description is a good place to discuss the failure/requirements, this is the ideal space for explaining why this design fixes the issue and why you designed it the way you did.

  • If there are important design decisions, particularly those that leverage functionality outside this change or were caused by existing limitations, they should be noted here.
  • If there is a design specification which relates to this PR, please provide a link to it here.
  • If the PR relates to an open issue, please link the issue to the PR by using the 'closes #issue_number' syntax.
  • If the PR relates to several open issue, make sure to use 'closes #issue_1, closes #issue_2, etc.' syntax.
  • If there are any dependencies (third-party or internal downstream) which are added or impacted, please note them here. And if their limitations informed your design, please note how they did so here (particularly if this information was not captured in a design specification).

Text goes here


Test Description

Please describe the tests you have written that verify the enhancement works or that the bug fix has been remedied. For a bug fix, please ensure a complete description of the failure is available here or in the issue such that a reviewer can verify that the testing is suitable. This is a good place to highlight any manual testing you may have had to perform.

Please describe any operating system limitations placed on the testing (if appropriate).

If your tests fixes any issue, please explain that in the comment you provided in the test file by referencing the issue within the comment and please record the mapping here e.g.

  • testA fixes issue 404
  • testB fixes issue 999

Text goes here


Documentation Impact

Please describe any changes to the documentation not captured above or, if made in a seperate pull request, please link to the corresponding PR or issue.

Text goes here


Other Details

If you have run any static analysis, including complexity analysis, or run coverage testing which has not been captured by an automated tool, please link or copy it here.

Text goes here


Pre-Request Checklist

  • The requirements of this pull request are fully captured in an issue or design specification and are linked and summarised in the description of this PR
  • No new warnings are generated
  • The documentation has been updated (or an issue has been created to track the corresponding change)
  • Methods and Tests are commented such that they can be understood without having to obtain additional context
  • This PR/Issue is labelled as a bug/feature/enhancement/breaking change
  • File dates have been updated to reflect modification date
  • This change conforms to the conventions described in the README

Please complete your pull request description and delete the instructional text before submitting.

Congratulations and thank you for making your contribution to neXtSIM_DG!

einola added 10 commits May 3, 2024 14:40
Define a new IOceanBoundary derived module for the OASIS ocean coupling.
 I simply copied and renamed the ConstantOceanBoundary module. Next
 steps are to include the OASIS calls themselves.
It belongs there, but it doesn't look like m_couplingArrays is actively
used.
setData should contain the initialisation calls, updateBefore the
receive calls and updateAfter the send calls.
Adds a setMetadata function to PrognosticData, which passes ModelMedata
to IAtmosphereBoundary and IOceanBoundary derived classes. This allows
them to call OASIS functions to initialise the coupling.

This can be done much better, but this commit acts a check-point, more
than anything else.
All the communicator and partitioning calls are common to ocean,
atmosphere, and wave coupling. So, I created OASISCoupled.hpp to keep
those calls. OASISCoupledOcean (and others later) then inherits from
this class and calls the parent class' setMetadata function in its own
setMetadata call. The rest of the child's setMetdata function then takes
care of the def_var and end_def calls
Only formatting changes.
We need to call oasis_c_terminate before MPI_Finalize is called. So, I
put this in the destructor of OASISCoupled.hpp and make sure that the
destructor of OASISCoupledOcean.hpp calls this. This should work, as the
 model object goes out of scope in main.cpp before MPI_Finalize is
 called.
This adds functionality to find OASIS to cmake and some oasis calls to
OASISCoupled.hpp. It compiles, but I haven't tested anything yet!
@einola einola marked this pull request as draft August 19, 2024 11:58
einola added 8 commits August 20, 2024 16:04
Move from all comments to actual code that compiles for the OASIS calls.
We still don't have any passing of information to the ocean - this
needs implementing. Most of the fields from the ocean to ice are
implemented already, but we're missing ssh and we need special handling
for mld (which is optional).
Something seems to have gone wrong in merge commit 1a155c9. Reverting
the error makes testTOPAZOcn run again.
OASIS needs to know how many seconds have passed since the experiment
started. I've added a simple internal counter for this, which is local
to OASISCoupled.hpp.
No other changes
This uses std::map<std::string, int> to link the namcople strings and
the values returned by oasis_c_def_var. Much better than what I had
before!
There are now a lot of options to set, so that it's flexible what you
put in the namcouple file.
I (optionally) pushed MLDKey to cplStringsIn before it could be
redefined by the configuration system.
@einola einola added the enhancement New feature or request label Aug 29, 2024
@einola einola added this to the 4 Ice-ocean coupled model milestone Aug 29, 2024
@einola
Copy link
Member Author

einola commented Aug 29, 2024

I set up this draft PR to help @andreapiacentini and me figure out how to get the OASIS coupling with NEMO working.

There's still a good deal of work left ...

einola added 2 commits August 29, 2024 11:33
Just so Andrea and Sophie see what I'm thinking
Just sorting out and gathering some output strings.
@einola einola requested a review from TomMelt September 9, 2024 11:44
The OASIS calls, init_comp, get_localcomm, terminate, def_partition, and
end_def can only be called once, so they must be moved out of the
OASISCoupled class and it's children. This makes the program structure
less nice, but I can't think of a better way to do it. It also leaves
OASISCoupled with only OASISTime and updateOASISTime(), but I suppose
that it's worth having anyway.
This needs to be figured out, but it's not a priority. Grid writing is
not necessary for the type of coupling we'll start with.
The implementation should be in the implementation file.
OASIS_CHECK_ERR doesn't work on oasis_c_put and oasis_c_get. Move
dimension0 and dimension1 to be global to the class.
Moves the calls to oasis_c_init_comp and oasis_c_get_localcomm to after
configuration, as suggested by Andrea. This makes for cleaner code and
opens the possibility to decide if we use oasis through the config file
(although this is not yet implemented).
Move #include <oasis_c.h> from ModelMetadata.hpp to ModelMetadata.cpp.
Remove an unused #include from ConfigOutput_test.cpp. Som automatic
clang-format changes as well.
Everything within the if (ENABLE_OASIS) clause everything should be
PUBLIC, but target_include_directories was PRIVATE. This is now
corrected.
It seems I misunderstood what they return and the role of kinfo (see
Andrea's comment to commit 7f8c587 on github).
As per Andrea's suggestion in a comment on commit be7905d on github.
As suggested by Andrea on github PR #641. I don't understand why the
case matters in the name of FindOASIS.cmake, but apparently it does for
him, although it doesn't for me. It should have been
${OASIS_DIR}/include all along.
A skeleton of a unit test in OASISCoupledOcean_test.cpp and some changes
 in the accompanying CMakeLists.txt to make it compile. Added
 MainMPI.cpp so that the test starts. We still need a namcouple file and
  the actual ins-and-outs of the test.
ModelMetadata metadata;

ocpl.setData(ModelState::DataMap());
ocpl.setMetadata(metadata);
Copy link

@andreapiacentini andreapiacentini Sep 13, 2024

Choose a reason for hiding this comment

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

The overridden setMetadata defines variables, yet it skips the oasis_c_enddef that should be issued by a generic call after all boundaries (ocean and or atmosphere) have finished defining variables.
I added

 OASIS_CHECK_ERR(oasis_c_enddef());

ModelComponent::getStore().registerArray(Protected::C_ICE, &cice, RO);
OASISCoupledOcean ocpl;
ModelMetadata metadata;

Choose a reason for hiding this comment

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

I had to add here

    const std::vector<int> partInfo = { OASIS_Serial, 1 };
    OASIS_CHECK_ERR(oasis_c_def_partition(
        &metadata.OASISPartitionId, OASIS_Serial_Params, &partInfo[0], OASIS_No_Gsize, compName.c_str()));

OASISCoupledOcean ocpl;
ModelMetadata metadata;

ocpl.setData(ModelState::DataMap());
Copy link

@andreapiacentini andreapiacentini Sep 13, 2024

Choose a reason for hiding this comment

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

I had to add a call to configure for getting the keys

    ocpl.configure();

and inside configure from OASISCoupledOcean.cpp I had to update the string lists:

    firstLayerDepth = Configured::getConfiguration(layerDepthConfigKey, FIRST_LAYER_DEPTH);

    cplStringsIn
      = { SSTKey, SSSKey, UOceanKey, VOceanKey, SSHKey };
    if (Configured::getConfiguration(exchangeFirstLayerConfigKey, EXCHANGE_FIRST_LAYER)) {
        cplStringsIn.push_back(MLDKey);
    }
    cplStringsOut
      = { TauXKey, TauYKey, TauModKey, EMPKey, QNoSunKey, QSWKey, SFluxKey, CIceKey };


ocpl.setData(ModelState::DataMap());
ocpl.setMetadata(metadata);
ocpl.updateBefore(TimestepTime());

Choose a reason for hiding this comment

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

There is a mismatch in updateBefore where VOceanKey is used twice.
With it fixed, I had a first running test !

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: In Progress
Development

Successfully merging this pull request may close these issues.

neXtSIM-NEMO coupling
2 participants