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

Prevent unnecessary recalculation of proxy values #336

Merged
merged 11 commits into from
Jul 17, 2024

Conversation

alexhernandezgarcia
Copy link
Owner

Context

While the Batch kept track of the availability of rewards and log-rewards, it did not store the proxy values, which are not needed for the computation of the loss. However, the proxy values were (re-)computed for logging purposes and for the Buffer. This was a waste of computation, especially if the proxy is expensive.

Main changes

  • The call of proxy.rewards() now allows returning the proxy values, alongside the rewards, if return_proxy is True.
  • After computing the rewards, the Batch now also stores the proxy values.
  • The GFlowNet agent now retrieves the proxy values from the Batch, without recomputing them.
  • The GFlowNet agent computes the (natural) rewards from the log-rewards and vice-versa.

Other changes

  • The buffer does not store the proxy values, since they are unused.
  • Some aspects of the Batch have been extended and refactored for better functionality and clarity.
  • The logger now logs stats about the log-rewards too.
  • Extended tests.
  • Extended docstring.

@ftherrien
Copy link
Collaborator

Thanks so much that was quick! I will "review" it later today

Copy link
Collaborator

@ftherrien ftherrien left a comment

Choose a reason for hiding this comment

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

Seems great to me. My understanding is that you mainly added a method to access self.proxy_values, made use of an existing method to get the rewards and added methods to check if the reward is log. Doc-strings are clear, internal variables renaming makes sense and tests seem relevant.

@alexhernandezgarcia alexhernandezgarcia merged commit aebc5f9 into main Jul 17, 2024
1 check passed
@alexhernandezgarcia alexhernandezgarcia deleted the proxy-in-batch-buffer-no-proxy branch July 17, 2024 15:29
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