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

Add Tests for vector input #607

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbusecke
Copy link
Contributor

@jbusecke jbusecke commented May 11, 2023


Tom: Suggested code improvements to prevent lots of bugs like this occurring:

  • Properly type hint all places where a vector could be passed,
  • Use the typing module's @overload decorator to type hint functions in an unambiguous way (i.e. scalar->scalar & vector->vector, but not vector->scalar),
  • Type hinting pad and its internals is the most important part, because that's the only place that vectors are treated meaningfully differently,
  • Other cases could potentially be handled by a @handle_vectors decorator wrapping methods, that unpacks, calls method, then re-wraps.
  • Test padding dask arrays in functions like diff_vector_2d,
  • Add an is_vector flag to any test dataset factory created (see #[FEATURE] Major refactor of the test suite #466 (comment))

@jbusecke
Copy link
Contributor Author

Added a test to check functionality with vector input.

This exposed another (but perhaps related bug) to #581:
From what I can see the vector dict is passed to _pad_basic as is, which should never happen.
Digging into the pad function now.

@jbusecke
Copy link
Contributor Author

Actually I think this bug is distinct (it happens inside the pad function, while the original bug occured in _rechunk_to_merge_in_boundary_chunks which happens after padding) from #581, but both are related to the vector syntax.
Trying to add a test that reproduces #581 first, before digging into pad.

Also I am not sure what the best testing strategy is here.

I think we need to parameterize the input (vector, non-vector) at least in a couple of places:

  • For all padding option on the stand-alone tests for padding
  • In the grid ufuncs tests, but do we need this for all of the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant