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

Speedup batch _compute_parents() #305

Merged
merged 2 commits into from
May 6, 2024
Merged

Speedup batch _compute_parents() #305

merged 2 commits into from
May 6, 2024

Conversation

carriepl
Copy link
Collaborator

@carriepl carriepl commented May 6, 2024

This PR speeds up the way the parents lists are reordered in Batch._compute_parents()

This is done by accumulating the indices needed for reordering in a dictionary rather than a list. This makes the reordering operations scale in O(N) instead of O(N^2) which is much more efficient for very large batches.

For the hcube/corners.yaml experiment (after the speedups to the Set environment in a different PR), it's about a 15-20% speedup and it's likely to be more for environments with longer sequences like the crystal env.

Copy link
Owner

@alexhernandezgarcia alexhernandezgarcia left a comment

Choose a reason for hiding this comment

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

Very cool!

I have compared this branch vs main with the continuous torus with trajectory length (fixed) 50 and batch size 100 and I get the following:

  • main: 48 seconds for 100 iterations
  • this branch: 28 seconds for 100 iterations

That's a great speedup! 😍

Command:

python main.py env=ctorus proxy=torus gflownet=trajectorybalance gflownet.optimizer.batch_size.forward=100 logger.do.online=False logger.test.period=-1 env.length_traj=50 env.buffer.train=null env.buffer.test=null

@alexhernandezgarcia
Copy link
Owner

alexhernandezgarcia commented May 6, 2024

There are a bunch of places where <list>.index(<item>) is used, so perhaps there are other low hanging fruits of this kind.

@alexhernandezgarcia alexhernandezgarcia merged commit 2167136 into main May 6, 2024
1 check passed
@alexhernandezgarcia alexhernandezgarcia deleted the speedup_batch branch May 6, 2024 20:37
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.

3 participants