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

Changes - hopefully improvements - on the design of the Cube environment #214

Merged
merged 55 commits into from
Sep 25, 2023

Conversation

alexhernandezgarcia
Copy link
Owner

@alexhernandezgarcia alexhernandezgarcia commented Sep 20, 2023

Cube design

  • The source state is not part of the object space any longer. Instead of being a regular state with value 0.0 for all dimensions, it has value -1 for all dimensions.
  • I have removed the variable max_val and simply hard code it to 1.0. There is no clear reason to have this additional complexity.
  • The actions now include an extra dimension to indicate whether the action is from or to source. This removes the need to comparison to [0.0, ...] with .isclose.
  • Instead of asserting that the state is within bounds, the action is marked as invalid and sampling can continue.

Code improvement

  • Since actions from source are simpler (Jacobian is one, conversion between relative/absolute is the identity, etc.), I have (hopefully) simplified the code of some methods.
  • I include one attribute epsilon in the environment to control the boundaries for clamping the inputs to log_prob.
  • The config distribution parameters are more intuitive:
    • Probabilities instead of logits for the Bernoulli distributions.
    • The actual alpha and beta that will go into the distribution (instead of the inputs to a sigmoid, then multiplied and shifted).

Test data set

  • I have also added an epsilon in the generation of the grid and uniform data sets. The tests I have run show that including states with dimensions equal or too close to the boundaries has an impact on the logprobs-based metrics. See wandb report.

Fixes

  • The variable name fixed_distribution in some config files was outdated and has been changed to the current fixed_distr_params

Other

  • Before @michalkoziarski complains, I acknowledge that I implemented the method _get_beta_params_from_mean_variance() which is eventually not used. But I have chosen to leave it for potential future use. Please excuse me the sacrilege.
  • I have conducted a partial analysis of the influence of the hyperparameters and added some preliminary conclusions in this wandb report.
  • New common test: forward and backward trajectories must be reversible (5b7ce4e)
    • Note that this test surfaces issues in the Crystal, LatticeParameters and Tree environments and is thus skipped for these environments until they get fixed.

…stead of logits and the actual alpha and beta that will go in the Beta policy
Copy link
Collaborator

@michalkoziarski michalkoziarski left a comment

Choose a reason for hiding this comment

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

This looks good to me, I added some minor comments.

config/experiments/ccube/ccube_pigeon_new.yaml Outdated Show resolved Hide resolved
gflownet/envs/cube.py Outdated Show resolved Hide resolved
gflownet/envs/cube.py Outdated Show resolved Hide resolved
gflownet/envs/cube.py Show resolved Hide resolved
gflownet/envs/cube.py Show resolved Hide resolved
gflownet/envs/cube.py Show resolved Hide resolved
@alexhernandezgarcia alexhernandezgarcia merged commit 419c9c8 into cube-sep23-allornone-oldtest Sep 25, 2023
1 check passed
@josephdviviano josephdviviano deleted the cube-abstract-source branch January 31, 2024 21:45
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