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

Added timeouts to optimizeLayoutAsync #20

Merged
merged 2 commits into from
Jun 8, 2019
Merged

Added timeouts to optimizeLayoutAsync #20

merged 2 commits into from
Jun 8, 2019

Conversation

drew-diamantoukos
Copy link
Contributor

Currently, the optimizeLayoutAsync method will recursively call itself for every step preventing the page from being able to update with any intermediate data retrieved from the epochCallback.

By adding this setTimeout, other code will be able to execute and allow the page to be more responsive while still ensuring that the promise is done resolving when all the steps are done.


I also attempted to implement a similar strategy for the 3 major loops inside the nNDescent function. However, that ultimately required implementing a similar sync/async split for initializeFit which, in, turn, required a better understanding of the codebase in order to properly refactor it. Not sure if that is on your radar, just wanted to let you know!

@cannoneyed
Copy link
Member

Hey thanks for sending this over!

Honestly, the best way to deal with all of this async / thread blocking stuff is to use a web worker, which I've begun looking into for the library itself. I and others have successfully loaded umap-js into a web worker (see this observable, but getting it to work right out of the box with the lib is a bit more challenging.

However I'll check this out and make sure it's all square, because it sounds like the right thing to do at least before any of this advanced stuff comes around.

FYI the review might take a bit longer from my end since I'm about to go on paternity, but I appreciate the PR!

@cannoneyed
Copy link
Member

Hey thanks for submitting this PR - I tested it out and it all looks fine, however the async method is a bit strange right now. Ideally, the callback function ought to return a promise so that we can more explicitly control the timing of the optimization compute, but I've actually done this in process with the manual step methods.... it's all a bit wonky right now, I admit, and I think the best solution would be a web worker based approach as I said.

Anyway, this all looks good / harmless for now so I'll approve and merge once you rebuild (just merged another PR before so there's merge conflicts in the lib)... Also, I should fix the lib folder being included in the github repo soon so we won't have to worry about this sort of thing.

Thanks again!

@drew-diamantoukos
Copy link
Contributor Author

Ah, yeah, web workers would be cool.

Thanks for reviewing!

@cannoneyed cannoneyed merged commit 6109ba8 into PAIR-code:master Jun 8, 2019
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