alexhernandezgarcia / gflownet

Generative Flow Networks - GFlowNet
https://gflownet.readthedocs.io/en/latest/
Apache License 2.0
167 stars 11 forks source link

Simplify state conversions (excludes Crystal environments) #247

Closed alexhernandezgarcia closed 9 months ago

alexhernandezgarcia commented 10 months ago

This PR simplifies state conversions in the following ways:

I have also taken the chance to add richer docstrings. I believe all other changes should be related to the above.

Note: the PR includes some deletion of files, which come from a different branch because I messed up things somehow... :sweat: Sorry for that!

alexhernandezgarcia commented 10 months ago

@carriepl I would like to have your advice on how to proceed with this PR. The situation is the following:

I am thinking that an auxiliary branch off main would do, but I wonder if there is any other better way.

carriepl commented 10 months ago

@carriepl I would like to have your advice on how to proceed with this PR. The situation is the following:

  • This branch was off main before main included the crystal's updates.
  • Therefore, this PR does not update the crystal's environments.
  • Therefore, if it is merged to main, then main will be broken, until the changes updating the crystal's environments are applied.

I am thinking that an auxiliary branch off main would do, but I wonder if there is any other better way.

Is there any obstacle to rebasing this branch (or a copy of this branch, to be safer) over the main and then updating the crystal environments in the rebased version of this branch?

alexhernandezgarcia commented 10 months ago

@carriepl I would like to have your advice on how to proceed with this PR. The situation is the following:

  • This branch was off main before main included the crystal's updates.
  • Therefore, this PR does not update the crystal's environments.
  • Therefore, if it is merged to main, then main will be broken, until the changes updating the crystal's environments are applied.

I am thinking that an auxiliary branch off main would do, but I wonder if there is any other better way.

Is there any obstacle to rebasing this branch (or a copy of this branch, to be safer) over the main and then updating the crystal environments in the rebased version of this branch?

That's about what I thought you could say. The only obstacle is that I don't feel super at ease with rebasing yet. Would you mind outlining what the steps should be?

josephdviviano commented 10 months ago

@carriepl I would like to have your advice on how to proceed with this PR. The situation is the following:

  • This branch was off main before main included the crystal's updates.
  • Therefore, this PR does not update the crystal's environments.
  • Therefore, if it is merged to main, then main will be broken, until the changes updating the crystal's environments are applied.

I am thinking that an auxiliary branch off main would do, but I wonder if there is any other better way.

Is there any obstacle to rebasing this branch (or a copy of this branch, to be safer) over the main and then updating the crystal environments in the rebased version of this branch?

That's about what I thought you could say. The only obstacle is that I don't feel super at ease with rebasing yet. Would you mind outlining what the steps should be?

OK one thing you could do is merge locally main -> conversion-simplify(from this branch on your machine, do git pull origin main), then after you've merged everything locally and committed those changes to the conversion-simplify branch, push them to conversion-simplify on github, and then finally complete the pull request here. There will no longer be merge conflicts with main.

I agree that rebasing will work but it's also confusing.

To save yourself heartache, make a zip archive of your local repo before you do the git pull in case the resulting merge conflicts are too confusing. I'm trying it now on my machine and it does not seem too bad, but I'm not familiar enough with the code to whip off the merge quickly without some research.

alexhernandezgarcia commented 9 months ago

So I am doing this the way it is easiest and safest to me.

  1. Create auxiliary branch off current main (conversions-simplify-merge).
  2. Resolve conflicts and merge this conversions-simplify into conversions-simplify-merge (copy of main)
  3. Implement rest of changes related to the simplification of state conversions (on crystal environments) in a new branch off conversions-simplify-merge and merge into main in a new PR (to be done).