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

Migrate CLI functionality to plugin layer subset 2 #2003

Merged
merged 46 commits into from
Jul 15, 2024

Conversation

cpelley
Copy link
Contributor

@cpelley cpelley commented May 29, 2024

Second/final chunk of plugin changes required by EPP.

Tasks

  • improver/cli/extract.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/freezing_rain.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/hail_fraction.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/hail_size.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/interpolate_using_difference.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/lightning_from_cape_and_precip.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/nbhood.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/phase_change_level.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/phase_probability.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/snow_splitter.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/vertical_updraught.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/wet_bulb_freezing_level.py
    • Working acceptance testing.
    • Unittesting coverage.
  • improver/cli/wet_bulb_temperature.py
    • Working acceptance testing.
    • Unittesting coverage.

Issues

@cpelley cpelley self-assigned this May 29, 2024
@cpelley cpelley changed the base branch from master to MIGRATE_CLI_FUNCTIONALITY_EPP May 29, 2024 15:02
@cpelley cpelley added Type:Maintenance blue_team MO review required PRs opened by non-Met Office developers that require a Met Office review labels May 29, 2024
@bayliffe bayliffe removed their assignment Jul 4, 2024
@cpelley
Copy link
Contributor Author

cpelley commented Jul 4, 2024

@bayliffe, directly relevant change in reflection of this work in MetOffice/dagrunner#40 (you need not review that, I mention only out of interest).

improver/nbhood/nbhood.py Outdated Show resolved Hide resolved
@@ -268,7 +269,7 @@ def _calculate_freezing_rain_probability(

return freezing_rain

def process(self, input_cubes: CubeList) -> Cube:
def process(self, *input_cubes: Union[Cube, CubeList]) -> Cube:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the type of each positional argument (note the asterisk). That is, each positional argument can be either a cube or cubelist. The documentation should then stipulate that we expect 3 cubes amongst whatever form of input is provided.
I'll flesh out the documentation here 👍

improver/precipitation_type/snow_splitter.py Show resolved Hide resolved
improver/wind_calculations/wind_direction.py Outdated Show resolved Hide resolved
improver/cli/extract.py Outdated Show resolved Hide resolved
@cpelley
Copy link
Contributor Author

cpelley commented Jul 5, 2024

Changes based on feedback from @bayliffe:

  • Moved ignore_error into the plugin of from improver/cli/extract.py.
    • Changes to improver_tests/utilities/cube_extraction/test_ExtractSubCube.py to reflect this change in behaviour to the plugin.
  • Recommended doc change to improver.utilities.complex_conversion.deg_to_complex.
  • Removed static methods deg_to_complex and complex_to_deg from WindDirection.
  • Migrated some testing from improver_tests/wind_calculations/wind_direction/test_WindDirection.py to new file improver_tests/utilities/complex_conversion/test_integration.py (roundtrip testing).
    • Changes to improver_tests/wind_calculations/wind_direction/test_WindDirection.py to ensure successful running of tests that remain.

The only thing I haven't done as I'm not quite following is what the issue is, is in regards to #2003 (comment)
However, I responded to what I think is a misunderstanding. *args is being passed to the function. The idea is just 1 parameter could be passed if its a cubelist, or 2 or 3 etc. I have verified that the documentation does highlight what data is actually needed to be provided.

@cpelley cpelley force-pushed the MIGRATE_CLI_FUNCTIONALITY_EPP_SUBSET2 branch from 80a83b2 to 53f2088 Compare July 5, 2024 12:05
@cpelley cpelley requested a review from bayliffe July 5, 2024 12:05
@cpelley
Copy link
Contributor Author

cpelley commented Jul 5, 2024

You happy to approve @bayliffe?

cheers

@cpelley cpelley force-pushed the MIGRATE_CLI_FUNCTIONALITY_EPP_SUBSET2 branch from 53f2088 to 5c9c797 Compare July 5, 2024 12:07
@cpelley
Copy link
Contributor Author

cpelley commented Jul 5, 2024

I suspect the CI failures with Sphinx are unrelated to this PR (a result of changes in the conda-forge conda environment, prob. needs something pinning or something unpinning).
Fix for CI in #2011

@cpelley cpelley changed the base branch from MIGRATE_CLI_FUNCTIONALITY_EPP to master July 5, 2024 15:59
@cpelley cpelley dismissed mo-robert-purvis’s stale review July 5, 2024 15:59

The base branch was changed.

@cpelley
Copy link
Contributor Author

cpelley commented Jul 5, 2024

I have merged in master into this branch and changed the target (since subset 1 was merged I forgot to change it here).

@cpelley
Copy link
Contributor Author

cpelley commented Jul 5, 2024

@mo-robert-purvis, you mind approving once more (it removed your approval after I changed the target to master).
cheers

SamGriffithsMO
SamGriffithsMO previously approved these changes Jul 8, 2024
@cpelley
Copy link
Contributor Author

cpelley commented Jul 9, 2024

I just merged master into this branch to resolve the CI failure.

@cpelley
Copy link
Contributor Author

cpelley commented Jul 11, 2024

Thanks both. @bayliffe, I have made the leadtimes optional once more. Thanks for double checking this for me.

Copy link
Contributor

@bayliffe bayliffe left a comment

Choose a reason for hiding this comment

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

Thanks Carwyn.

@cpelley cpelley merged commit 351ee40 into master Jul 15, 2024
16 checks passed
@cpelley cpelley deleted the MIGRATE_CLI_FUNCTIONALITY_EPP_SUBSET2 branch July 15, 2024 09:28
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blue_team MO review required PRs opened by non-Met Office developers that require a Met Office review Type:Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants