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

WIP Switch to data time cycling to support multiple models and multiple case studies or trials #765

Open
wants to merge 90 commits into
base: main
Choose a base branch
from

Conversation

jfrost-mo
Copy link
Member

@jfrost-mo jfrost-mo commented Jul 30, 2024

Fixes #750

WIP in title is just to stop accidental merge before all three reviews.

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change. Documentation for data time cycling #814
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

GitHub Copilot was used in the PR.

@jfrost-mo jfrost-mo added the enhancement New feature or request label Jul 30, 2024
@jfrost-mo jfrost-mo self-assigned this Jul 30, 2024
Copy link
Contributor

github-actions bot commented Jul 30, 2024

Coverage

@jfrost-mo jfrost-mo linked an issue Aug 1, 2024 that may be closed by this pull request
@jfrost-mo jfrost-mo changed the title Switch to datetime cycling, support multiple models, etc. Switch to data time cycling, support multiple models, etc. Aug 8, 2024
@jfrost-mo jfrost-mo force-pushed the 750_data_time_cycling branch 3 times, most recently from 2133d21 to d6b08fe Compare August 22, 2024 09:53
@jfrost-mo jfrost-mo changed the base branch from main to 614_robust_plot_index_writing August 22, 2024 14:53
Base automatically changed from 614_robust_plot_index_writing to main August 23, 2024 06:45
@jfrost-mo jfrost-mo force-pushed the 750_data_time_cycling branch 4 times, most recently from b1d6ab6 to 62ec811 Compare August 23, 2024 15:47
@jfrost-mo jfrost-mo changed the title Switch to data time cycling, support multiple models, etc. Switch to data time cycling to support multiple models and multiple case studies or trials Aug 23, 2024
@jfrost-mo jfrost-mo marked this pull request as ready for review August 27, 2024 15:33
@jfrost-mo
Copy link
Member Author

This work is now ready to be reviewed.

The output_dir was in the wrong place, so it was making a POST request.
This prevents needing to rely on external hosts for these. I did need to drop
the example commands to fetch the resources via curl, but they were probably
confusing anyway.
They are not implemented yet, and can be re-added when they are.
The operator now requires all arguments to either be numbers, or all be None
if you don't want to constrain the area.

The test has been mildly improved, so at least it checks for both of the
named functions on the coordinates, though it still doesn't test that they
do anything.
@jfrost-mo
Copy link
Member Author

@SGallagherMet I think I've addressed all of your comments now. Could you give it a second look?

Copy link

@SGallagherMet SGallagherMet 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 to me now.

@jfrost-mo
Copy link
Member Author

@JorgeBornemann just needs your portability review now. For reference, the tutorial branch we are using for the workshop next week is the same as this one, minus a couple of the more recent commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request full_review Requires a technical, scientific, and portability review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cycle over data time
3 participants