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.

Sign up for GitHub

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

Jump to bottom

Basic calendar functionality #87

Merged
merged 3 commits into from
Jul 14, 2022
Merged

Basic calendar functionality #87

merged 3 commits into from
Jul 14, 2022

Conversation

LenkaNovak
Copy link
Collaborator

@LenkaNovak LenkaNovak commented Jul 13, 2022 •
edited
Loading

PULL REQUEST

Purpose and Content

Adds the usage of Dates and a calendar_callback macro, which allows evaluations on a particular date. This is a prerequisite PR for BC specification at different times from file.

Benefits and Risks

  • benefit:
    • enables AMIP BC specification
    • designed as a general callback, so will be useful for monthly diagnostics aggregation, message showing and logging
  • risk: step-wise operations may slow down code - benchmarks in place

Linked Issues

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.

This was referenced Jul 13, 2022
Closed
Merged
Closed
Merged
jb-mackay
jb-mackay reviewed Jul 13, 2022
View reviewed changes
experiments/AMIP/moist_mpi_earth/coupler_utils/calendar_timer.jl
current_date(t)
Return the model date
"""
current_date(t) = date0 + Dates.Second(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Going forward, it will be best if date0 is not a global variable. The question then is, what is the natural place to store this? There is some code in src regarding a clock that could be expanded or changed to support this!

Copy link
Collaborator Author

@LenkaNovak LenkaNovak Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, there are a few places in the coming PRs where global variables are used (e.g. landmask). I can make an issue, so we don't forget to come back to this when we revisit the interface. (#92 )

jb-mackay
jb-mackay approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@jb-mackay jb-mackay left a comment

Choose a reason for hiding this comment

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

Looks good @LenkaNovak - what are the scenarios which we will use these data callbacks? Can this be built around the time kept in the integrator?

@LenkaNovak LenkaNovak mentioned this pull request Jul 13, 2022
Closed
10 tasks
@LenkaNovak
Copy link
Collaborator Author

Looks good @LenkaNovak - what are the scenarios which we will use these data callbacks? Can this be built around the time kept in the integrator?

Thanks for the review @jb-mackay ! 🚀 Yes, I would say that once we re-introduce the interface where the coupler has its own way of keeping track of t, we should def make the callback a function of that.

@LenkaNovak
Copy link
Collaborator Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 14, 2022

Build succeeded:

@bors bors bot merged commit 1646d99 into main Jul 14, 2022
@bors bors bot deleted the calendar-timer branch July 14, 2022 00:52
bors bot added a commit that referenced this pull request Jul 15, 2022
@bors @LenkaNovak
Merge #89
eceeec6 
89: Separate regridding from file reader r=LenkaNovak a=LenkaNovak

# PULL REQUEST

## Purpose and Content

This PR re-organises the regridding `coupler_utils`, so that regridding is separate from reading the regridded output from `TempestRemap`. 

- note that mesh generation with the space-filling curve breaks the regridding order required in `TempestRemap`. This was fixed with specifying a `regrid_space` using the previous method of mesh generation. However, this should be generalized in the future.  

## Benefits and Risks
- benefit: this will enable a monthly callback for temporal interpolation 
- risk: few more lines to the interface (though this will be removed by the following PR)

## Linked Issues
- SDI: #74 
- Design: #88

## Dependent PRs
- [x] do not merge before  #87 

## PR Checklist
- [x] This PR has a corresponding issue OR is linked to an SDI.
- [x] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [x] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [x] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [x] I linted my code on my local machine prior to submission OR N/A.
- [x] Unit tests are included OR N/A.
- [x] Code used in an integration test OR N/A.
- [x] All tests ran successfully on my local machine OR N/A.
- [x] All classes, modules, and function contain docstrings OR N/A.
- [x] Documentation has been added/updated OR N/A.


Co-authored-by: LenkaNovak <lenka@caltech.edu>
bors bot added a commit that referenced this pull request Jul 25, 2022
@bors @LenkaNovak
Merge #90
62eb27b 
90: Monthly updating of BCs from file r=LenkaNovak a=LenkaNovak

# PULL REQUEST

## Purpose and Content
This PR adds functionality to collect BC field information in a struct `..._info`, which is then updated each month with a new file read of the regridded data. In the next PR `..._info` will contain two Fields read from a file, which will be used for the daily interpolation.

## Benefits and Risks
- benefit: prerequisite for the monthly to daily interpolation
- risk: changes the way the BCs are read, but most of it is abstracted

## Linked Issues
- SDI: #74 
- Design: #88

## Dependent PRs
- no not merge before  
  - [x] #87 
  - [x]  #89
  - [x]  #93 
 
## Coupling loop performance
- the macro is comparable to using functions (though `@time` larger variability). The pros and cons for this method are [here](#95), are will be revisited post AMIP (and well as including the unit test in the same file).
- in either case the monthly update does not cause noticeable performance change from the previous commit for the full AMIP setup. 

## PR Checklist
- [x] This PR has a corresponding issue OR is linked to an SDI.
- [x] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [x] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [x] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [x] I linted my code on my local machine prior to submission OR N/A.
- [x] Unit tests are included OR N/A.
- [x] Code used in an integration test OR N/A.
- [x] All tests ran successfully on my local machine OR N/A.
- [x] All classes, modules, and function contain docstrings OR N/A.
- [x] Documentation has been added/updated OR N/A.

Co-authored-by: LenkaNovak <lenka@caltech.edu>
@LenkaNovak LenkaNovak mentioned this pull request Jul 28, 2022
Merged
10 tasks
bors bot added a commit that referenced this pull request Jul 29, 2022
@bors @LenkaNovak
Merge #105
ec8de33 
105: Daily interpolation from monthly data (v2) r=LenkaNovak a=LenkaNovak

# PULL REQUEST
- Update from #91

## Purpose and Content
Adds capability to interpolate linearly between two monthly `Fields`. The demo uses prerequisite PRs outlined in #88 to obtain monthly data. 

## Benefits and Risks
- benefit: last piece for enabling specifying temporarily varying boundary conditions from a file.
- risk: performance slow-down - check are in place (see below)

## Linked Issues
- SDI: #74 
- Design: #88

## Dependent PRs
- no not merge before  #87 ,  #89 and #90

## Performance of the final solution 
- time loop only over added infrastructure of #88 : adding 0.24s per simulation day at `dt = 200s`

## PR Checklist
- [x] This PR has a corresponding issue OR is linked to an SDI.
- [x] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A.
- [x] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy).
- [x] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
- [x] I linted my code on my local machine prior to submission OR N/A.
- [x] Unit tests are included OR N/A.
- [x] Code used in an integration test OR N/A.
- [x] All tests ran successfully on my local machine OR N/A.
- [x] All classes, modules, and function contain docstrings OR N/A.
- [x] Documentation has been added/updated OR N/A.

Co-authored-by: LenkaNovak <lenka@caltech.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jb-mackay jb-mackay jb-mackay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LenkaNovak @jb-mackay