Preprocessor refactoring

[younik]

I believe preprocessors should not be part of the Env class, and they should kept separate.

1. These lines in env examples don't scale well:
   

   torchgfn/src/gfn/gym/hypergrid.py

   Lines 58 to 72 in 6f132a8

   	if preprocessor_name == "Identity":
   	preprocessor = IdentityPreprocessor(output_dim=ndim)
   	elif preprocessor_name == "KHot":
   	preprocessor = KHotPreprocessor(height=height, ndim=ndim)
   	elif preprocessor_name == "OneHot":
   	preprocessor = OneHotPreprocessor(
   	n_states=self.n_states,
   	get_states_indices=self.get_states_indices,
   	)
   	elif preprocessor_name == "Enum":
   	preprocessor = EnumPreprocessor(
   	get_states_indices=self.get_states_indices,
   	)
   	else:
   	raise ValueError(f"Unknown preprocessor {preprocessor_name}")

2. It seems they are not used inside the env class, but they are just passed to GFNModule, which is a better location.

torchgfn/testing/test_gflownet.py

Lines 40 to 46 in 6f132a8

	env = Box()
	module = BoxPFMLP(
	hidden_dim=32, n_hidden_layers=2, n_components=1, n_components_s0=1
	)
	estimator = DiscretePolicyEstimator(
	module, n_actions=2, preprocessor=env.preprocessor
	)

I believe they should be instantiated by the user and eventually passed to GFNModule.

[josephdviviano]

Yes this makes sense to me. The GFNModule would be able to handle raw and pre-processes states using whatever preprocessor is passed to it. Joseph Viviano @josephdviviano <https://twitter.com/josephdviviano> viviano.ca

…

On Sun, Nov 17, 2024 at 10:31 AM Omar Younis ***@***.***> wrote: I believe preprocessors should not be part of the Env class, and they should kept separate. 1. These lines in env examples don't scale well: https://github.com/GFNOrg/torchgfn/blob/6f132a869902584c06b7f8054e76dda696d78c94/src/gfn/gym/hypergrid.py#L58-L72 2. It seems they are not used inside the env class, but they are just passed to GFNModule, which is a better location. https://github.com/GFNOrg/torchgfn/blob/6f132a869902584c06b7f8054e76dda696d78c94/testing/test_gflownet.py#L40-L46 I believe they should be instantiated by the user and eventually passed to GFNModule. — Reply to this email directly, view it on GitHub <#220>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7TL2X27CXAQSGOXTIC5HT2BCZFNAVCNFSM6AAAAABR6BK5QSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGY3DMMBQHAZDENQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[saleml]

By the way, this was the case 2 years ago. In fact, back then, we used to define an Estimator as a GFModule + Preprocessor object: https://github.com/GFNOrg/torchgfn/blob/7ac7de2a39e6bf6ca36985a3762fcef63d57bb56/gfn/estimators.py

This big commit, which is actually more of a code restructure than a mere commit, changed that. I had put the preprocessor in the environment.

Given the current structure of the code base, and the fact that Estimators are not a thing anymore, I agree with with your suggestion.