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

Add remapping to coupler puts & gets #58

Merged
merged 8 commits into from
Jun 30, 2022
Merged

Add remapping to coupler puts & gets #58

merged 8 commits into from
Jun 30, 2022

Conversation

jb-mackay
Copy link
Contributor

@jb-mackay jb-mackay commented Jun 15, 2022

Purpose and Content

Adds remapping to the coupler put! and get calls. Implements these calls in the sea breeze simulation

Benefits and Risks

Hides explicitly calling remapping operations from the user.

Next steps

The initialization phase needs reworking and reordering in order to facilitate the auto-construction of the remap operators.

Linked Issues

#44

PR Checklist

  • This PR has a corresponding issue OR is linked to an SDI.
  • I have followed CliMA's codebase contribution and style guidelines OR N/A.
  • I have followed CliMA's documentation policy.
  • I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
  • I linted my code on my local machine prior to submission OR N/A.
  • Unit tests are included OR N/A.
  • Code used in an integration test OR N/A.
  • All tests ran successfully on my local machine OR N/A.
  • All classes, modules, and function contain docstrings OR N/A.
  • Documentation has been added/updated OR N/A.

@jb-mackay jb-mackay mentioned this pull request Jun 15, 2022
10 tasks
@jb-mackay
Copy link
Contributor Author

Currently it is not possible to map to an exchange or intermediary grid -- any field put! in the coupler lives on the space of the simulation that puts it there.

@jb-mackay
Copy link
Contributor Author

Currently it is not possible to map to an exchange or intermediary grid -- any field put! in the coupler lives on the space of the simulation that puts it there.

This is because of the current way the mapping is set up which uses a tag (write_sim) to specify the source space/sim when remapping. This tag also (as the name suggests) indicates which simulation has write permissions to that coupled field.

Perhaps the right solution is to remove the double usage of write_sim and add another tag like "space_name" which would be part of the remap operator name.

@jb-mackay
Copy link
Contributor Author

This PR sets up several follow up PRs:

  1. Documentation updates:
    i. Change example used to the sea breeze as this is checked by buildkite and now a cleaner example
    ii. Add DocStringExtensions to improve doc quality
  2. Add a coupler_get! method that accepts a field that will be modified/filled by the coupler_get call
  3. ClimaCore: introduce AbstractRemap abstract type, with concrete subtypes including LinearRemap, IdentityRemap, RLLRemap, etc.
  4. Update coupler_put! to include a remap operation. This creates symmetry and more flexibility for what grid the coupling takes place on.
  5. Auto-build remap operators during init phase

@jb-mackay jb-mackay requested a review from kmdeck June 22, 2022 05:50
ocn_to_atm = Operators.LinearRemap(atm_boundary, ocn_domain)
lnd_to_atm = Operators.LinearRemap(atm_boundary, lnd_domain)
maps = (
atmos_to_ocean = Operators.LinearRemap(ocn_domain, atm_boundary),
Copy link
Member

Choose a reason for hiding this comment

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

minor, but we should be consistent between using "atm" and "atmos", "land" and "lnd, etc.

Copy link
Collaborator

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

This looks very neat, thanks @jb-mackay ! I added a couple of comments, both of which we can discuss when we do the interface revamp. I think this can be merged.


# init coupler fields and maps
coupler = CouplerState()
coupler_add_field!(coupler, :T_sfc_ocean, ocean.integrator.u.T_sfc; write_sim = ocean)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way of asserting that the correct models write into the right vars could be via multiple dispatch, which would allow specifying the same name to one variable in different domains (e.g. T_sfc(::atm_sim), T_sfc(::ocn_sim) etc). We can discuss this during the interface 🍐 next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm worried that that would lead to a large number of methods for these shared variables. I think it would be better to have fewer methods that can give us flexibility. Let's discuss further.

struct SimulationA{T} <: ClimaCoupler.AbstractSimulation
data::T
end
ClimaCoupler.name(::SimulationA) = :simA
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we could get away with avoiding the (simA, simB, ...) assignment, e.g. using Symbol? Again, something we can address during the next iteration. Noted down to discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it would be good for this to work for an arbitrary number of sims, each of which has it's own user-defined tag. Maybe the solution will become clearer next week, when we focus on the interface! :)

@jb-mackay
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 30, 2022

@bors bors bot merged commit c50d9e7 into main Jun 30, 2022
@bors bors bot deleted the bm/putgetremap branch June 30, 2022 17:21
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.

3 participants