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

move CoupledSim to Interfacer #455

Merged
merged 1 commit into from
Oct 5, 2023
Merged

move CoupledSim to Interfacer #455

merged 1 commit into from
Oct 5, 2023

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Oct 2, 2023

Purpose

closes #333

Components:

  • move the code
  • revamp unit tests

QA

  • ensure there is no behavioral change in Buildkites (AMIP paperplots (namely the 32 day run), conservation check for slabplanet, performance in build_history)

Questions:

  • src/CoupledSimulations/coupled_simulation.jl CoupledSimulation vs Interfacer.CoupledSimulation - why is this type defined in two places?
  • timestepping.md refers to ClimaCoupler.CoupledSimulation - which of the two types is this?
  • should we keep AbstractSimulation defined in Interfacer? This is also defined in src/CoupledSimulations/coupled_simulation.jl as well

  • I have read and checked the items on the review checklist.

@juliasloan25
Copy link
Member Author

@LenkaNovak could you help me understand the questions I have in the PR description?

  • src/CoupledSimulations/coupled_simulation.jl CoupledSimulation vs Interfacer.CoupledSimulation - why is this type defined in two places?
  • timestepping.md refers to ClimaCoupler.CoupledSimulation - which of the two types is this?
  • should we keep AbstractSimulation defined in Interfacer? This is also defined in src/CoupledSimulations/coupled_simulation.jl as well

@LenkaNovak
Copy link
Collaborator

LenkaNovak commented Oct 3, 2023

Sure:

    1. src/CoupledSimulations/ files were placeholders for the Simulations-like interface. Since we're not going to implement this in the near future, we can probably remove it (as long as it doesn't clash with the sea_breeze example. If it does, then we can revamp this as part of a different PR that updates the ClimaCore examples). Same goes for src/CouplerState/
    1. timestepping.md refers to this outdated code and should be removed. There will be more similar files to clean up for the release, so we should go over these on Monday.
    1. similar to point 1. The AbstactSimulaiton in src/CoupledSimulations/ is outdated, but it may break the sea breeze case if we remove it without modifying the example's run script.

@juliasloan25
Copy link
Member Author

I think getting rid of the src/CoupledSimulations and src/CoupledState folders will change enough code that I want to do it in a separate PR: #458

@juliasloan25 juliasloan25 force-pushed the js/coupledsim branch 2 times, most recently from e7fb7b5 to fc284b5 Compare October 5, 2023 00:15
@juliasloan25
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 5, 2023
455: move CoupledSim to Interfacer r=juliasloan25 a=juliasloan25



Co-authored-by: Julia Sloan <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 5, 2023

Canceled.

@juliasloan25
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 5, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 2970957 into main Oct 5, 2023
6 checks passed
@bors bors bot deleted the js/coupledsim branch October 5, 2023 16:13
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.

Move CoupledSimulation to Interfacer
2 participants