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

Use CSET read operator to load test data, and improve various fragile tests #1076

Merged
merged 10 commits into from
Jan 30, 2025

Conversation

jfrost-mo
Copy link
Member

@jfrost-mo jfrost-mo commented Jan 24, 2025

A fairly wide ranging set of test improvements. The main focus is ensuring we are using the CSET read operators to load data, so it is representative of what CSET will actually be handling. The only place that remains using iris.load is the tests for the read operators themselves.

There are also several robustness improvements to tests, to check the desired property directly rather than some tangential thing, such as the repr().

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.
  • 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.

@jfrost-mo jfrost-mo added the cleanup Non-functional improvement label Jan 24, 2025
@jfrost-mo jfrost-mo self-assigned this Jan 24, 2025
Copy link
Contributor

github-actions bot commented Jan 24, 2025

Coverage

@jfrost-mo jfrost-mo force-pushed the use_cset_read_operator_in_tests branch 3 times, most recently from 55f7370 to 726e37d Compare January 27, 2025 14:22
@jfrost-mo jfrost-mo marked this pull request as ready for review January 27, 2025 14:27
@jfrost-mo jfrost-mo requested a review from daflack January 27, 2025 16:35
Copy link
Contributor

@daflack daflack 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, one comment to ensure backwards compatibility as one of the convection tests has lost full coverage.

tests/operators/test_convection.py Show resolved Hide resolved
Copy link
Contributor

@daflack daflack left a comment

Choose a reason for hiding this comment

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

As now loaded by CSET the time coordinate should not appear on its own, as all cubes given a realization coordinate. Therefore, this part of the operator that is "not tested" can be removed when the convection code is updated to run with LFRic. Please create an issue.

@jfrost-mo jfrost-mo requested a review from daflack January 28, 2025 17:05
They were very brittle due to relying on the serialisation of a
method signature. These have been converted to directly check the
concerned properties of the cubes.
Also use more robust checking on single point regrid tests and use
range rather than long list of expected points.
@jfrost-mo jfrost-mo force-pushed the use_cset_read_operator_in_tests branch from a2055ff to 993c6f6 Compare January 28, 2025 17:07
@jfrost-mo
Copy link
Member Author

I've rebased to fix some merge conflicts, and improve the data loading in the convection tests in commit 993c6f6, which is the only new once since your last review. I'd appreciate another review of those additional changes before I merge this.

Copy link
Contributor

@daflack daflack left a comment

Choose a reason for hiding this comment

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

Happy with those changes.

@jfrost-mo jfrost-mo merged commit cd271a5 into main Jan 30, 2025
8 checks passed
@jfrost-mo jfrost-mo deleted the use_cset_read_operator_in_tests branch January 30, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non-functional improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants