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

with_flatten: make the layer input/output types symmetric #821

Merged
merged 6 commits into from
Jan 10, 2023

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Dec 13, 2022

The idea of the with_flatten layer is that it flattens a nested sequence, passes it to the wrapped layer, and then unflattens the output of the wrapped layer.

However, the layer was asymmetric in that it passes a list to the wrapped layer, but expects back an XP array. This breaks composition with other Thinc layers, such as with_array.

This change makes with_flatten symmetric, in that the inputs/outputs of the with_flatten and the wrapped layer are symmetric.

It seems that this layer is not used in Thinc or spaCy, so maybe it never worked correctly? At any rate, I needed to flatten a nested list in distillation with with_flatten(with_array(...)) in distillation and found that it doesn't actually work.

The idea of the `with_flatten` layer is that it flattens a nested sequence, passes
it to the wrapped layer, and then unflattens the output of the wrapped layer.

However, the layer was asymmetric in that it passes a list to the wrapped layer,
but expects back an XP array. This breaks composition with other Thinc layers,
such as `with_array`.

This change makes `with_flatten` symmetric, in that the inputs/outputs of the
`with_flatten` and the wrapped layer are symmetric.
Co-authored-by: Adriane Boyd <adrianeboyd@gmail.com>
Copy link
Contributor

@kadarakos kadarakos left a comment

Choose a reason for hiding this comment

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

Maybe its me, but since with_flatten is in a deep learning library my initial assumption was that it will call xp.flatten on an ArrayXd making it Array1d and it will reshape it back. Should it dynamically either flatten a list or a tensor? Should it be able to flatten any nesting level with lists just like any order tensor in xp.flatten? Or we just have it like this and add it into the documentation that this is flattening lists of lists (one level only)? Or is it just me who made the assumption? Don't want to derail the PR just mentioning that flatten for me invoked something different.

thinc/layers/with_flatten_v2.py Outdated Show resolved Hide resolved
@danieldk
Copy link
Contributor Author

danieldk commented Jan 9, 2023

Maybe its me, but since with_flatten is in a deep learning library my initial assumption was that it will call xp.flatten on an ArrayXd making it Array1d and it will reshape it back. Should it dynamically either flatten a list or a tensor? Should it be able to flatten any nesting level with lists just like any order tensor in xp.flatten? Or we just have it like this and add it into the documentation that this is flattening lists of lists (one level only)? Or is it just me who made the assumption? Don't want to derail the PR just mentioning that flatten for me invoked something different.

These are good questions, and I agree that our naming it not really in line with other deep learning libraries, though it is kinda the normal FP definition.

At any rate, this PR is just for fixing the existing implementation which just flattens one level of lists. If we want something more advanced, it should probably have a different name to avoid confusion (and a separate PR).

@adrianeboyd adrianeboyd merged commit f7a819f into explosion:master Jan 10, 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.

4 participants