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

Store the last update between optimize() calls #229

Merged
merged 4 commits into from
Feb 14, 2023

Conversation

dkobak
Copy link
Contributor

@dkobak dkobak commented Jan 28, 2023

Fix #228 and add a corresponding test.

@dkobak
Copy link
Contributor Author

dkobak commented Jan 28, 2023

I don't understand why the pickling test fails here.

@pavlin-policar
Copy link
Owner

The tests likely fail because the pickling/unpickling process is quite brittle. I'll take a look. Thanks for tracking this down. This is definitely something we want to implement.

@dkobak dkobak changed the title store updates between optimize calls Store the last update between optimize() calls Feb 11, 2023
@dkobak
Copy link
Contributor Author

dkobak commented Feb 11, 2023

When the test fails, it happens in pickle.dumps(embedding), and the error is triggered in __reduce__():

    957         new_state = state[2] + (
--> 958             self.optimizer,
    959             self.affinities,
    960             self.gradient_descent_params,

AttributeError: 'TSNEEmbedding' object has no attribute 'optimizer'

What is weird, is that embedding.optimizer actually does exist, and moreover, calling embedding.__reduce__() also works fine, but calling pickle.dumps(embedding) triggers the above error...

@dkobak
Copy link
Contributor Author

dkobak commented Feb 14, 2023

I FOUND THE BUG!!! Only took me two weeks :-) The update array needed to be converted from TSNEEmbedding to np.array. All checks pass now.

@pavlin-policar
Copy link
Owner

That's wonderful news! That must have been a pain to track down. This looks good to me, and, again, thanks for finding and fixing this bug :)

@pavlin-policar pavlin-policar merged commit 153ad71 into pavlin-policar:master Feb 14, 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.

Bug: running optimize() multiple times produces different result compared to running it once
2 participants