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

Core _io.concatenate() may fail due to case when one of the cubes is scalar - this fixes that #1715

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Sep 5, 2022

Description

Closes #1716

The introduction of scalar cubes in iris complicates a number of things, including our advanced concatenation mechanism - it is possible, as seen from the test case changed in this PR, that along the path of concatenation as we do it, one of the processed cubes that we use as intermediary laison to be scalar - then the whole show crumbles. I am not a fan of scalar cubes as they are defined in iris, but that's for another day. Anyway, this PR fixes that particular corner case, and adds a test case (converts the previous test case to a more meaningful one, that is).

All started from @bouweandela 's comment - cheers, Bouwe! 🍺


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@valeriupredoi valeriupredoi added the bug Something isn't working label Sep 5, 2022
@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Merging #1715 (c6c9274) into main (ed81a3c) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1715      +/-   ##
==========================================
- Coverage   91.52%   91.47%   -0.06%     
==========================================
  Files         206      206              
  Lines       11202    11204       +2     
==========================================
- Hits        10253    10249       -4     
- Misses        949      955       +6     
Impacted Files Coverage Δ
esmvalcore/preprocessor/_io.py 85.08% <100.00%> (-2.32%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@valeriupredoi
Copy link
Contributor Author

hold yer horses, I'll improve the test coverage - @bouweandela why is our test coverage test passing even if the coverage is shrinking? Go home test, you drunk 🍺

@bouweandela
Copy link
Member

The test coverage test tests that all the changed code is covered by tests. So if you delete some code that is covered by tests from a file with less than 100% coverage (such as the debug log call you removed), the relative amount of covered code decreases, while the changed code is fully covered because there isn't any.

cubes = iris.cube.CubeList([c1_delta, cubes[1]])
logger.debug("Attempting concatenatenation of %s with %s",
Copy link
Member

Choose a reason for hiding this comment

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

Did you want to remove this?

Copy link
Contributor Author

@valeriupredoi valeriupredoi Sep 6, 2022

Choose a reason for hiding this comment

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

not at all, I was being silly - cheers for pointing it out, bud, will reinstate 👍

@valeriupredoi
Copy link
Contributor Author

The test coverage test tests that all the changed code is covered by tests. So if you delete some code that is covered by tests from a file with less than 100% coverage (such as the debug log call you removed), the relative amount of covered code decreases, while the changed code is fully covered because there isn't any.

makes sense, cheers, Bouwe! V, go home, you're drunk 😁

@valeriupredoi
Copy link
Contributor Author

cheers for the review @bouweandela 🍺 Your tests should all go fine now in your Dasky PR

@valeriupredoi valeriupredoi merged commit 2397641 into main Sep 6, 2022
@valeriupredoi valeriupredoi deleted the fix_concatenation_for_scalar_cube branch September 6, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_io.concatenate()` may fail if extracted laison cube is scalar
2 participants