Skip to content

Initial implementation of shape checking method #35

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

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Nov 8, 2023

Would like to add tests and some usage of this

@ksunden ksunden changed the title Initial implementeation of shape checking method Initial implementation of shape checking method Nov 14, 2023
@ksunden
Copy link
Member Author

ksunden commented Nov 15, 2023

Some thoughts:

  • Do we wish to return something other than true/false? e.g. return the values of the variables and error if incompatible

    • This is more similar to e.g. np.broadcast_shapes, which returns the resultant shape and errors if they are incompatible
    • May have some oddities when it comes to broadcast=True where a variable may be omitted or have to have a fill value when all inputs that use it are broadcastable
  • There is some thought towards using e.g. flat variadic args that are then passed through itertools.batched rather than as tuples.

    • I think that that would be better for the case of using interactively, but probably actually worse in most production use. I think most production use will tend to do something like:
expected = {
    "x": ("M",),
    "y": ("N",),
    "z": ("M", "N",),
}
found = cont.describe()
if Desc.check_shapes(*[(expected[i], found[i]) for i in expected
]):
    ...
else:
   raise ...

And the extra unpacking to flatten that further is actually more verbose at that point.

  • Instead of paired tuples, perhaps we accept the two dictionaries directly, which also validates the keys then.

  • This usage pattern is also the reason why it currently has expected as a "raw" shape and found as a Desc object, which I don't love from an API POV, but think it needlessly complicates the code using check_shapes to require Desc objects for expected.

  • Options here:

    • accept either tuples or Description objects (on both ends) and sort it out internal to check_shapes
    • Force consistency (in either direction), which would mean forcing expected to be Desc objects or doing found[i].shape to get the shape tuple out.
      • Some interplay with the idea of using dict instead of 2-tuple pairs here, as that would make the change to enforce consistency more complicated, since you'd just be passing the output of describe and so would have to do a whole comprehension rather than a small edit to an existing comprehension.

@tacaswell tacaswell marked this pull request as ready for review December 6, 2023 16:28
@tacaswell tacaswell merged commit dc8a96b into matplotlib:main Dec 6, 2023
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.

2 participants