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

Feature/sparse pooling #43

Merged
merged 13 commits into from Mar 23, 2022
Merged

Feature/sparse pooling #43

merged 13 commits into from Mar 23, 2022

Conversation

Flegyas
Copy link
Contributor

@Flegyas Flegyas commented Mar 22, 2022

This PR adds a new pooling method for subwords (well, two, but the inefficient one is there only for benchmarking purposes).
The sparse one is necessary for contexts where we want to enable CUDA determinism since scatter methods do not support it.

The script benchmark.py compares them, but I think that there is some mismatch in the approaches since these are the results (GPU: NVIDIA 2060S | CPU: AMD 3700X):

scatter == sparse (allclose with atol=1e-07): False
scatter == inefficient (allclose with atol=1e-07): False
sparse == inefficient (allclose with atol=1e-07): True
scatter 23.960102558135986s
sparse 23.518492221832275s
inefficient 24.366436004638672s

I wrote the "inefficient" pooling method as a control one, and it seems like the scatter method is not matching its results.
I think the mismatch can be traced to something weird happening with the padded positions, but I didn't investigate further.

I could very well have implemented both the control and the sparse methods wrongly, so please double-check everything!

And thank you for the library, it is truly useful!

@Riccorl Riccorl merged commit fdf8d7b into Riccorl:dev Mar 23, 2022
@Riccorl
Copy link
Owner

Riccorl commented Mar 23, 2022

Thanks for the PR! I will compare them in some downstream task to check that everything works.

If everything is fine, I will clean up some stuff and release it (2.1 I guess).

@Flegyas Flegyas deleted the feature/sparse-pooling branch March 23, 2022 10:43
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

2 participants