search

GFNOrg / torchgfn

A modular, easy to extend GFlowNet library
https://torchgfn.readthedocs.io/en/latest/
Other
228 stars 28 forks source link

Refactoring ideas regarding the `preprocessor` #194

Open hyeok9855 opened 1 week ago

hyeok9855 commented 1 week ago

After reviewing the codes, I brought two possible refactoring ideas.

  1. Move the parameter preprocessor in GFNModule to Sampler. This could separate env from GFNModule (since the preprocessor is defined in env) and pass the preprocessed states to the GFNModule. One drawback is that the input of GFNModule would not be State anymore.

  2. There are two preprocessor files: src/gfn/preprocessors.py and src/gfn/gym/helpers/preprocessors.py. If this is not intended, it would be better to merge them into one file.

These are naive suggestions, so please feel free to comment, and let's discuss!

josephdviviano commented 1 week ago

Thanks for your ideas!

  1. For 1 - I'm not sure where the states would be passed? I think we could also explore integrating the sampler completely within the GFNModule although I'm worried this might be a footgun down the line if some method requires that modularity. @saleml thoughts?
  2. I don't love src/gfn/gym/helpers in general. I feel like the "helpers" should either be integrated into the main library or the individual gym files, and the whole folder should be removed.
hyeok9855 commented 6 days ago

I think we could also explore integrating the sampler completely within the GFNModule

Yes! Let's discuss this in #195 about it :)