Simplify state conversions (excludes Crystal environments) #247

alexhernandezgarcia · 2023-11-10T01:33:59Z

This PR simplifies state conversions in the following ways:

state*2oracle has been removed as well as other mentions to oracle. In short, references to the oracle are now unified into references to the proxy.
statebatch2proxy and statetorch2proxy are now unified by states2proxy
statebatch2policy and statetorch2policy are now unified by states2policy
state2proxy has been removed from all environments because it is enough with the implementation in the base environment, which calls states2proxy.
policy2state has been removed.

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

I have run the sanity checks and everything seems fine.
Tests are passed locally.

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

Update dev main with public main

…to main

…s still there.

… is still there.

… old code is still there.

…still there.

…old code is still there.

…there.

alexhernandezgarcia · 2023-12-07T22:40:54Z

@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.

carriepl · 2023-12-08T16:46:39Z

@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 · 2023-12-08T20:35:14Z

@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

Love this, net -1000 lines of code is a good PR any day of the week!

I wonder whether some of the environment specific conversion logic would be further abstracted into the base class or not, but it does not seem cumbersome to write these few lines of code, so it's not a priority.

Good stuff!

josephdviviano · 2023-12-08T20:47:04Z

gflownet/utils/common.py

    else:
        return torch.tensor(x, dtype=torch.long, device=device)


 def tint(x, device, int_type):
    if isinstance(x, list) and torch.is_tensor(x[0]):
-        return torch.stack(x).type(int_type).to(device)
+        return torch.stack(x).to(device=device, dtype=int_type)


Just a note - if you're constantly doing this, it's a performance hazard. As I'm sure you're aware preallocating and filling an empty tensor will perform better than stacking lists, but of course it can lead to more complex code. Might be a good subject for a different PR.

You're right. Rather for another PR indeed.

josephdviviano · 2023-12-08T21:08:17Z

@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 · 2023-12-09T18:50:26Z

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

Create auxiliary branch off current main (conversions-simplify-merge).
Resolve conflicts and merge this conversions-simplify into conversions-simplify-merge (copy of main)
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).

nikita-0209 and others added 30 commits April 11, 2023 11:58

checkout files from test_merge_cont_mf

737cda8

remove comments

afdb762

Update to a slightly faster version of get_parents()

e7cd7a5

Merge branch 'main_seq' into sequence-envs

b57c695

Move sequence config files into new dir seqs/

7567fe3

Move sequence proxy config files into new dir seqs/

2ba9ae4

Move sequence env *.py files into new dir seqs/

e607efc

wip: refactor of parent sequence class

690fde0

Replace original README with info about the new private repository.

beed5df

Merge remote-tracking branch 'public/main' into main-public

40c7f33

WIP: few changes to sequence base

9a4829d

Merge remote-tracking branch 'public/sequence-envs' into main

02ab997

Finished __init__ for now.

b4919d6

Docstring of get_action_space.

8ce60ab

Merge pull request #2 from alexhernandezgarcia/main-public

d280222

Update dev main with public main

Merge branch 'main' of github.com:alexhernandezgarcia/gflownet-dev in…

1c17674

…to main

Remove env.oracle and leave proxy only

8b0b621

Grid: states2proxy, states2policy; temporary state because old code i…

0088ee4

…s still there.

docstring

c83d8b3

Tetris: states2proxy, states2policy; temporary state because old code…

04bb59f

… is still there.

Refactoring of tfloat, tlogn, tint and tbool

41962f9

Typo in typing and correct docstrings

e446262

Cube: states2policy; temporary state because old code is still there.

be67b5c

Fix typos

89ebdaf

Continuous Tori: states2proxy, states2policy; temporary state because…

43bf2ea

… old code is still there.

Alanine Dipeptide: states2proxy; temporary state because old code is …

7593578

…still there.

Missing imports

72a31ea

Discrete Torus: states2proxy, states2policy; temporary state because …

f7a8d2b

…old code is still there.

Tree: remove mention to oracle

7201991

Composition: states2proxy; temporary state because old code is still …

127a017

…there.

alexhernandezgarcia added 14 commits October 31, 2023 11:44

Discrete torus: Remove state2policy and output of policy to float

ea16fc2

Update env.reward() in base

55b8ed3

Test batch: statebatch2policy -> states2policy

1e71a6d

Envs: remove state2proxy

a50cd7d

Remove reward_torchbatch because it is unused

3880e48

Add TODO

03308a9

statetorch2 -> states2

5e3c765

Fix

388816c

Delete files not relevant to branch

14e30c1

Delete files not relevant to branch

ce38e3f

Delete files not relevant to branch

88d3fa6

Delete files not relevant to branch

f0e3feb

Fix tensor comparison

cc634cf

Add typing in returns of state2policy and state2proxy of base env.

11d5fc4

alexhernandezgarcia requested review from nikita-0209 and josephdviviano and removed request for nikita-0209 November 22, 2023 20:01

alexhernandezgarcia mentioned this pull request Nov 23, 2023

Improvements to MillerIndices environment #252

Merged

4 tasks

josephdviviano approved these changes Dec 8, 2023

View reviewed changes

alexhernandezgarcia changed the base branch from main to conversions-simplify-merge December 9, 2023 18:22

Resolve conflicts with main

fe105d1

alexhernandezgarcia changed the title ~~Simplify state conversions~~ Simplify state conversions (excludes Crystal environments) Dec 9, 2023

alexhernandezgarcia mentioned this pull request Dec 9, 2023

Simplify state conversions (Crystal environments) #269

Merged

alexhernandezgarcia merged commit fe105d1 into conversions-simplify-merge Dec 9, 2023
1 check failed

alexhernandezgarcia mentioned this pull request Dec 22, 2023

Simplify state conversions - combination #270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify state conversions (excludes Crystal environments) #247

Simplify state conversions (excludes Crystal environments) #247

alexhernandezgarcia commented Nov 10, 2023

alexhernandezgarcia commented Dec 7, 2023

carriepl commented Dec 8, 2023

alexhernandezgarcia commented Dec 8, 2023

josephdviviano left a comment

josephdviviano Dec 8, 2023

alexhernandezgarcia Dec 8, 2023

josephdviviano commented Dec 8, 2023 •

edited

Loading

alexhernandezgarcia commented Dec 9, 2023 •

edited

Loading

Simplify state conversions (excludes Crystal environments) #247

Simplify state conversions (excludes Crystal environments) #247

Conversation

alexhernandezgarcia commented Nov 10, 2023

alexhernandezgarcia commented Dec 7, 2023

carriepl commented Dec 8, 2023

alexhernandezgarcia commented Dec 8, 2023

josephdviviano left a comment

Choose a reason for hiding this comment

josephdviviano Dec 8, 2023

Choose a reason for hiding this comment

alexhernandezgarcia Dec 8, 2023

Choose a reason for hiding this comment

josephdviviano commented Dec 8, 2023 • edited Loading

alexhernandezgarcia commented Dec 9, 2023 • edited Loading

josephdviviano commented Dec 8, 2023 •

edited

Loading

alexhernandezgarcia commented Dec 9, 2023 •

edited

Loading