Skip to content

Conversation

@alexandrelarouche
Copy link
Contributor

Hi,

I have implemented two simple gym environments which I find useful when experimenting with GFNs.

First one is simply an append MDP where there is a set of items, and any item can be added to the set at any time. Exit is allowed at any time except at the initial state. An instance may look like the following MDP (without the terminal state to lighten the figure).

image

The second environment is a Perfect Binary Tree MDP, where there is a bijection between trajectories and terminating states. Action 0 goes left, action 1 goes right. One could easily extend this environment to non-binary trees. We can discuss if it is necessary or interesting for torchgfn to have a non-binary extension environments. An instance of Perfect Binary Tree may look like:
image

@alexandrelarouche alexandrelarouche marked this pull request as ready for review February 17, 2025 22:13
@saleml
Copy link
Collaborator

saleml commented Feb 18, 2025

Thank you for the PR @alexandrelarouche
Could you please pre-commit run --all before you commit, then re-push once it succeeds?
For pyright, if you can't solve the issues, you can ignore the lines with a specific comment (that we already used throughout the codevase)

@alexandrelarouche
Copy link
Contributor Author

alexandrelarouche commented Feb 18, 2025

Hey @saleml,

My bad, I did run the pre-commit yesterday, but apparently I was negligent in noticing the pyright checks failed. This does open up an avenue of discussion though - it seems strange to require declaring a States type hint in a DiscreteEnv instead of a DiscreteStates. Is that something we could aim to fix or an inherent limitation of pyright's understanding of inheritance?

Thanks for being so pro-active on the repo 🙂 .

@saleml
Copy link
Collaborator

saleml commented Feb 19, 2025

Is that something we could aim to fix or an inherent limitation of pyright's understanding of inheritance?
This is definitely something that can and should be fixed.

The first fix should be, indeed, to change all mentions of States to DiscreteStates in the DiscreteEnv class.

There is an open issue (#235) for this type of changes, and we're hoping a pedantic power-user of the library would some day appear and solve it. Perhaps LLMs can do that soon. Otherwise, when/if time permits, I'll do it...

Regarding the two environments you added, are there some published benchmarks, or tests for them?
If you look at the test_scripts.py file, we give some examples of minimal performances to reach for the implemented environments. This should future-proof the environments. I strongly recommend a similar test for each of your environments (of the type: if you run TB with BS 128 using this type of preprocessor, you would get this performance).

Do you think this is something feasible with the current code?

@alexandrelarouche
Copy link
Contributor Author

alexandrelarouche commented Feb 19, 2025

Regarding the two environments you added, are there some published benchmarks, or tests for them?
If you look at the test_scripts.py file, we give some examples of minimal performances to reach for the implemented environments. This should future-proof the environments. I strongly recommend a similar test for each of your environments (of the type: if you run TB with BS 128 using this type of preprocessor, you would get this performance).
Do you think this is something feasible with the current code?

As of now, I do not know of specific benchmarks for these specific environments. I use these environment mostly on paper to study GFN behaviors, as they can easily map to the applicative settings I am interested in. I am certain they map to some other applicative scenarios that were studied before, but they offer an abstraction layer above them. As such, I can hardly provide a satisfying test that would show my implementation is correct.

However, as I use them in my own research, I can provide the tests once the papers are out. In the meantime, I understand that this may not be satisfactory for a PR, and we can leave this open if you wish.

Copy link
Collaborator

@saleml saleml left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. As you might have observed, there have been major chances in the master/main branch of the repo since you started this PR. Besides the minor comment on the code, could you:

  • Merge master into your branch
  • Add a test for each of your environments, in test_scripts.py, to ensure that the current gflownet losses work on your environment (you can hardcode a specific value for the L1 distance desired for example, ensuring that it goes down after 10 TB SGD steps, or something like that). You can take inspiration from the other tests in that file.
    ?
    I'll merge your branch into master right after that



class SetAddition(DiscreteEnv):
"""Append only MDP, similarly to what is described in Remark 8 of Shen et al. 2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a one or two line explanation of this environment, e.g. states, actions... ?

@josephdviviano
Copy link
Collaborator

@alexandrelarouche I'll take this over. Thanks so much for the work!

@saleml saleml merged commit 3a4157a into GFNOrg:master Apr 11, 2025
3 checks passed
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