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

LFRic extension to vertical profile #638

Merged
merged 32 commits into from
Jul 1, 2024

Conversation

Sylviabohnenstengel
Copy link
Contributor

@Sylviabohnenstengel Sylviabohnenstengel commented May 23, 2024

Fixes #637

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.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Added an entry to the top of docs/source/changelog.rst
  • Conda lock files have been updated if dependencies have changed.
  • Said whether Generative AI, such as GitHub Copilot, has been used in this PR.
  • Marked the PR as ready to review.

Copy link
Contributor

github-actions bot commented May 23, 2024

Coverage

@Sylviabohnenstengel
Copy link
Contributor Author

first requires review of Um version for vertical profile #494

@Sylviabohnenstengel Sylviabohnenstengel added enhancement New feature or request science Scientific capabilities labels May 23, 2024
@jfrost-mo jfrost-mo changed the base branch from main to vertical_profiles_of_area_averaged_fields_494 May 23, 2024 13:13
@jfrost-mo jfrost-mo changed the title 637 lfric extension to vertical profile LFRic extension to vertical profile May 23, 2024
@jfrost-mo jfrost-mo force-pushed the vertical_profiles_of_area_averaged_fields_494 branch from 7ae4310 to c53e945 Compare May 24, 2024 07:50
Base automatically changed from vertical_profiles_of_area_averaged_fields_494 to main May 29, 2024 09:07
@jfrost-mo jfrost-mo force-pushed the 637_lfric_extension_to_vertical_profile branch from 08884b0 to 2e93eb1 Compare June 24, 2024 11:15
@jfrost-mo
Copy link
Member

The multilevel data we currently have is UMified, so we should first check with Dasha about getting some SLAM only data.

@Sylviabohnenstengel
Copy link
Contributor Author

Dasha has provided slam only data. The lfric data vertical coordinate is full_levels and not model_level_number like in the um data. recipe, constraint and plot code have been adjusted for that.

@Sylviabohnenstengel
Copy link
Contributor Author

need to keep in mind that some lfric data migth operate on half_levels and others on full_levels.

@Sylviabohnenstengel
Copy link
Contributor Author

Sylviabohnenstengel commented Jun 25, 2024

recipe is working, but units are unknown for lfric data
image

@Sylviabohnenstengel Sylviabohnenstengel marked this pull request as ready for review June 25, 2024 16:04
@Sylviabohnenstengel
Copy link
Contributor Author

GitHub Copilot was used to inform a few lines of code.

@Sylviabohnenstengel
Copy link
Contributor Author

@jfrost-mo Do I need to add use of github copilot in the changelog?

@jfrost-mo
Copy link
Member

You've added a comment to the PR, so that covers that. As this is the first PR that will be merged using GitHub Copilot, I'll also add a note to the README in the PR.

@jfrost-mo jfrost-mo force-pushed the 637_lfric_extension_to_vertical_profile branch from ebc8c55 to 33cc79e Compare June 27, 2024 09:26
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

I've suggested a bunch of things. I'm happy to hop on a call and go through them if that would be helpful.

cset-workflow/meta/rose-meta.conf Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Sylviabohnenstengel Sylviabohnenstengel left a comment

Choose a reason for hiding this comment

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

all approved bar the two changes that suggest to use "pressure" as default vertical coordinate as I think we will switch to model levels as lfric doe snot yet provide pressure levels. As we tend to explicitly pass this into the function via recipe it should not be an issue.

Sylviabohnenstengel and others added 25 commits July 1, 2024 11:32
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
This does what we have been trying to do, in having a single operator
that can handle all levels.
I added pressure to make it absolutely clear t=it generates constraints for all types of vertical levels
@jfrost-mo jfrost-mo force-pushed the 637_lfric_extension_to_vertical_profile branch from 477c4ca to 10f69ee Compare July 1, 2024 10:32
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Happy to approve now.

@jfrost-mo jfrost-mo merged commit 1091d80 into main Jul 1, 2024
7 checks passed
@jfrost-mo jfrost-mo deleted the 637_lfric_extension_to_vertical_profile branch July 1, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request science Scientific capabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add lfric capability to vertical profile branch and for spatial plotting on plevels
3 participants