Mobt 157 nbhood refactor consolidate unit tests part1 #1665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
One unit test needs updating following the rebase after the previous PR was merged. I'll fix this along with any other review comments after the testbed. |
gavinevans
requested changes
Feb 9, 2022
There was a problem hiding this comment.
Thanks @fionaRust 👍
As a stepping stone towards the refactoring of the neighbourhood processing plugins, I think that this is fine. I've added a couple of very minor comments, one of which is already noted on the ticket.
For another reviewer: The size of the diff in this PR is much larger than necessary, as improver_tests/nbhood/circular_kernel/test_GeneratePercentilesFromACircularNeighbourhood.py and improver_tests/nbhood/nbhood/test_GeneratePercentilesFromANeighbourhood.py are almost identical.
improver_tests/nbhood/nbhood/test_GeneratePercentilesFromANeighbourhood.py
Outdated
Show resolved
Hide resolved
Kat-90
reviewed
Feb 10, 2022
| @@ -213,6 +213,7 @@ def test_circular_neighbourhood(self): | |||
| self.higher_threshold, | |||
| neighbourhood_method, | |||
| self.radii, | |||
| weighted_mode=True, | |||
| )._calculate_convective_ratio(self.cubelist, self.threshold_list) | |||
| self.assertArrayAlmostEqual(result, expected) | |||
|
|
|||
There was a problem hiding this comment.
Is this test now similar to the one described below? The one below says 'set to true' in the doc string but is actually setting to false, where this one is setting to true.
There was a problem hiding this comment.
Also, based on the name and doc string, it feels like these two tests are testing the same thing that the unit tests in test_NeighbourhoodProcessing (lines 80 and 98) are testing? But maybe the test is actually testing something different. If so, it might be worth changing the doc string/name to be more specific to this test.
There was a problem hiding this comment.
I've updated the docstring in the test below, as you're right it wasn't correct.
These tests are testing a method on the DiagnoseConvectivePrecipitation that uses neighbourhood processing, so these tests are testing something different to the test_NeighbourhoodProcessing. See:
https://github.com/metoppv/improver/blob/master/improver/precipitation_type/convection.py#L143
I'm also not too worried about these tests as the plugin is going to be removed in https://github.com/metoppv/mo-blue-team/issues/214
There was a problem hiding this comment.
Thanks for explaining and thanks for the updated doc string.
…factor_consolidate_unit_tests_part1 * feature_branch_nbhood_refactor: Adds a filter to the combine CLI for mismatching realizations (metoppv#1656) Reduce the memory requirements for read-the-docs (metoppv#1672) Further doc-building fixes. (metoppv#1671) DOC Fix intersphinx links for docs (metoppv#1668)
Codecov Report
@@ Coverage Diff @@
## feature_branch_nbhood_refactor #1665 +/- ##
==================================================================
- Coverage 98.16% 98.14% -0.02%
==================================================================
Files 110 110
Lines 10085 10085
==================================================================
- Hits 9900 9898 -2
- Misses 185 187 +2
Continue to review full report at Codecov.
|
gavinevans
approved these changes
Feb 15, 2022
Kat-90
reviewed
Feb 16, 2022
| @@ -213,6 +213,7 @@ def test_circular_neighbourhood(self): | |||
| self.higher_threshold, | |||
| neighbourhood_method, | |||
| self.radii, | |||
| weighted_mode=True, | |||
| )._calculate_convective_ratio(self.cubelist, self.threshold_list) | |||
| self.assertArrayAlmostEqual(result, expected) | |||
|
|
|||
There was a problem hiding this comment.
Thanks for explaining and thanks for the updated doc string.
improver/nbhood/square_kernel.py
Outdated
| f" weighted_mode provided: {weighted_mode}, " | ||
| f"neighbourhood_method provided: {neighbourhood_method}." | ||
| ) | ||
| print(msg) |
There was a problem hiding this comment.
Think this print needs removing.
fionaRust
commented
Feb 16, 2022
fionaRust
added a commit
to fionaRust/improver
that referenced
this pull request
Feb 16, 2022
…factor_consolidate_unit_tests_rebased * feature_branch_nbhood_refactor: Mobt 157 nbhood refactor consolidate unit tests part1 (metoppv#1665) Adds a filter to the combine CLI for mismatching realizations (metoppv#1656) Reduce the memory requirements for read-the-docs (metoppv#1672) Further doc-building fixes. (metoppv#1671) DOC Fix intersphinx links for docs (metoppv#1668)
fionaRust
added a commit
to fionaRust/improver
that referenced
this pull request
Feb 23, 2022
…factor_tidy_CLIs * feature_branch_nbhood_refactor: Mobt 157 nbhood refactor consolidate unit tests rebased (metoppv#1664) Mobt 157 nbhood refactor consolidate unit tests part1 (metoppv#1665) Adds a filter to the combine CLI for mismatching realizations (metoppv#1656) Reduce the memory requirements for read-the-docs (metoppv#1672) Further doc-building fixes. (metoppv#1671) DOC Fix intersphinx links for docs (metoppv#1668) Mobt 157 nbhood refactor sort out base classes (metoppv#1653) Modifies wxcode check_tree utility function to report issues with unreachable nodes (metoppv#1637) remove cycle (metoppv#1657) Minor edits to remove raising unnecessary warnings. (metoppv#1646) Change pandas DataFrame.at to DataFrame.loc (metoppv#1655) MOBT-154: Reunification of wx decision trees (metoppv#1639) Consolidate scale parameter usage across EMOS and ECC (metoppv#1642) Adds handling of a model-id-attr to wxcode-modal (metoppv#1634)
bayliffe
pushed a commit
that referenced
this pull request
Apr 6, 2022
* Mobt 157 nbhood refactor code consolidation (#1647) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Responding to review comments Updating docstrings, removing unused code and dealing with unexpected neighbourhood_methods * Minor changes after review comments * Mobt 157 nbhood refactor sort out base classes (#1653) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Removed extra layer of base class from neighbourhood plugins * Simplified process method on base neighbourhood processing plugin * Further simplification of the BaseNeighbourhoodProcesing class Moved BaseNeighbourhoodProcessing unit tests. Removed slicing over time and suport for multi-time cubes * Moved old test process test back to BaseNeighbourhoodProcessing to make diff smaller * Updated plugin docstrings and handling of mask data in percentile nbhood * Minor fix to unit test * Responding to 1st review comments * Remove print statement * Corrected name and docstring for unit test * Removed unused option from function definition * Mobt 157 nbhood refactor consolidate unit tests part1 (#1665) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Minor changes following review comments * Remove print statement * Mobt 157 nbhood refactor consolidate unit tests rebased (#1664) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Fixed mistakes introduced when merging in feature branch * Responding to first review comments * Responding to second review comments * improved test docstring * Mobt 157 nbhood refactor simplify calculate neighbourhood (#1680) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Fixed mistakes introduced when merging in feature branch * Responding to first review comments * Responding to second review comments * Removed uncessary code and tried to make _calculate_neighbourhood clearer * Mobt 157 nbhood refactor tidy clis (#1679) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Removed extra layer of base class from neighbourhood plugins * Simplified process method on base neighbourhood processing plugin * Further simplification of the BaseNeighbourhoodProcesing class Moved BaseNeighbourhoodProcessing unit tests. Removed slicing over time and suport for multi-time cubes * Moved old test process test back to BaseNeighbourhoodProcessing to make diff smaller * Updated plugin docstrings and handling of mask data in percentile nbhood * Minor fix to unit test * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Updeated land-sea and top nbhood processing to work with circular * Fixed merge errors * Updating docstrings and unit tests names following review * Removed confusing docstring info * Making docstring flow better * Parametrizing tests * Removed commented out code * Mobt 157 nbhood refactor move plugins (#1691) * Moved all neighbourhoood plugins into one module. * Updated checksums after acceptance test removed in previous PR. * Updated test coverage of GeneratePercentilesFromANeighbourhood * Removed promotion of single point percentile and realization dimensions to dim coords * Added square as the default neighbourhood shape to neighbourhood CLIS. (#1695)
MoseleyS
pushed a commit
to MoseleyS/improver
that referenced
this pull request
Aug 22, 2024
* Mobt 157 nbhood refactor code consolidation (metoppv#1647) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Responding to review comments Updating docstrings, removing unused code and dealing with unexpected neighbourhood_methods * Minor changes after review comments * Mobt 157 nbhood refactor sort out base classes (metoppv#1653) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Removed extra layer of base class from neighbourhood plugins * Simplified process method on base neighbourhood processing plugin * Further simplification of the BaseNeighbourhoodProcesing class Moved BaseNeighbourhoodProcessing unit tests. Removed slicing over time and suport for multi-time cubes * Moved old test process test back to BaseNeighbourhoodProcessing to make diff smaller * Updated plugin docstrings and handling of mask data in percentile nbhood * Minor fix to unit test * Responding to 1st review comments * Remove print statement * Corrected name and docstring for unit test * Removed unused option from function definition * Mobt 157 nbhood refactor consolidate unit tests part1 (metoppv#1665) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Minor changes following review comments * Remove print statement * Mobt 157 nbhood refactor consolidate unit tests rebased (metoppv#1664) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Fixed mistakes introduced when merging in feature branch * Responding to first review comments * Responding to second review comments * improved test docstring * Mobt 157 nbhood refactor simplify calculate neighbourhood (metoppv#1680) * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Fixed mistakes introduced when merging in feature branch * Responding to first review comments * Responding to second review comments * Removed uncessary code and tried to make _calculate_neighbourhood clearer * Mobt 157 nbhood refactor tidy clis (metoppv#1679) * Remove __repr__ methods from all neighbourhood plugins * Moved circular neighbourhood processing into the square kernel * Renamed SquareNeighbourhood plugin to Neighbourhood * Added missing docstring * Removed extra layer of base class from neighbourhood plugins * Simplified process method on base neighbourhood processing plugin * Further simplification of the BaseNeighbourhoodProcesing class Moved BaseNeighbourhoodProcessing unit tests. Removed slicing over time and suport for multi-time cubes * Moved old test process test back to BaseNeighbourhoodProcessing to make diff smaller * Updated plugin docstrings and handling of mask data in percentile nbhood * Minor fix to unit test * Make options for neighbourhood sum more consistent. * Consolidate percentile from neighbourhood unit tests * Made weighted mode only a valid option if you are using circular kernel * Updated __init__unit tests * Added tests for _calculate_neigbourhood method * Added unit tests for process method * Remove all unnecessary tests * Updated incorrect docstrings * Updeated land-sea and top nbhood processing to work with circular * Fixed merge errors * Updating docstrings and unit tests names following review * Removed confusing docstring info * Making docstring flow better * Parametrizing tests * Removed commented out code * Mobt 157 nbhood refactor move plugins (metoppv#1691) * Moved all neighbourhoood plugins into one module. * Updated checksums after acceptance test removed in previous PR. * Updated test coverage of GeneratePercentilesFromANeighbourhood * Removed promotion of single point percentile and realization dimensions to dim coords * Added square as the default neighbourhood shape to neighbourhood CLIS. (metoppv#1695)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part one of this PR #1664