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

Adds ActionIndex and Harmonize use of action-masks #129

Merged
merged 25 commits into from
Apr 11, 2024

Conversation

julienroyd
Copy link
Contributor

Masks were used somewhat ad-hoc here and there. Here we centralise masking in GraphActionCategorical(), specifically:

  • renamed masks in GraphActionCategorical to action_mask (to avoid confusion with done-masking, molecule validity masking, goal-conditioning masking, etc.)
  • made logits and masks private attributes of GraphActionCategorical
  • remove _mask() application from GraphTransformerFragEnvelopeQL and GraphTransformerGFN
  • removed masking from GraphSampler (masks applied in property setters of GraphActionCategorical._logits)

We also replaced the action-tuples Tuple[int, int, int] action type, row and column by a named-tuple ActionIndex to the code and typing more intuitive and readable.

* renamed masks in GraphActionCategorical to action_mask (to avoid confusion with done-masking, molecule validity masking, goal-conditioning masking, etc.)
* made logits and masks private attributes of GraphActionCategorical
* remove _mask() application from GraphTransformerFragEnvelopeQL and GraphTransformerGFN
* removed masking from GraphSampler (masks applied in property setters of GraphActionCategorical._logits)
…-masking i.e. masked_logit * 0. -> 0. (un-masked))
… outside the class definition and avoid silent bugs due to unintended default configs
@julienroyd julienroyd force-pushed the julien-harmonize-use-of-masks branch from ce4ec3d to 5f953c6 Compare April 4, 2024 14:08
@julienroyd julienroyd marked this pull request as ready for review April 4, 2024 18:08
@julienroyd julienroyd requested a review from bengioe as a code owner April 4, 2024 18:08
@julienroyd
Copy link
Contributor Author

Can reproduce previous results with goal-conditioning and 4 objectives:
image

Copy link
Collaborator

@bengioe bengioe left a comment

Choose a reason for hiding this comment

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

LGTM, just two questions.

src/gflownet/envs/graph_building_env.py Show resolved Hide resolved
src/gflownet/algo/graph_sampling.py Outdated Show resolved Hide resolved
@julienroyd julienroyd merged commit 4d13758 into trunk Apr 11, 2024
4 checks passed
@julienroyd julienroyd deleted the julien-harmonize-use-of-masks branch April 11, 2024 19:53
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