Naming suggestion regarding GFNModule

[hyeok9855]

I found that GFNModule (here) is a parent of *Estimator sub-classes. Then, why not naming it as GFNEstimator?

With the new name, we could also avoid confusion from the two similar file names, src/gfn/modules.py and src/gfn/utils/modules.py, by changing the former to src/gfn/estimators.py.

[josephdviviano]

I like GFNModule since it's subclasses nn.Module.

Going another way, perhaps we could rename the Estimators to Modules - however, not all Estimators need to be modules. They can for example be a tabular lookup, if you want.

[hyeok9855]

Since not all *Estimators are nn.module, why not remove nn.Module from the parent of the abstract GFNModule and instead inherit nn.Module in the subclasses that have learnable parameters?

If so, we can rename GFNModule to GFNEstimator or GFNPolicy (I prefer the latter).

I believe the renaming will help to reduce possible confusion regarding src/gfn/modules.py and src/gfn/utils/modules.py.

[josephdviviano]

Well, all GFNs that are useful will have learnable parameters.

A GFN is not just one policy (always) - sometimes it is multiple policies (forward / backward, for example). So I'm not sure about GFNPolicy.

I agree that we should handle src/gfn/utils/modules.py -- perhaps something like src/gfn/utils/estimator_components.py?

This way we have a clear distinction between Estimators and GFNModules - which are fairly distinct concepts and I don't think we win anything by merging them.

[hyeok9855]

I see. There is the ScalarEstimator that models state flows, so GFNPolicy is not appropriate.

As far as I could tell, the GFNModule is a superclass for $F(s)$, $P_F(s'|s)$, and $P_B(s|s')$, which are the key learnable components of GFlowNets (thus, GFN "Module" seems nice).

Still, I believe the design can be improved in the following senses:

1. GFNModule as a superclass of learnable parameters?: This is not true for now since another key learnable component $Z$ for TB is not included in GFNModule. Then, would it be better to include Z in GFNModule, also? I don't think so, since the input for $Z$ is different from the others, we're hesitant to include it in GFNModule.
2. How much $F$ and $P_F, P_B$ have in common?: I think the only common part is forward() part, which has common preprocessor. If so, what about treat $F$ separately from $P_F$ & $P_B$ (also, we may need to move the common preprocessor to GFlowNet class), and use the term GFNPolicy for a superclass of $P_F, P_B$?? - In this case, we may not have superclass for $F$, just like $Z$.

[josephdviviano]

I don't fully follow your logic. I want users to be able to compose all of these elements together in novel ways, at will, and allowing one to assemble these components under a unified GFNModule is the easiest way to do it. Furthermore ensuring the base class is a nn.Module avoids all sorts of nasty silent bugs with pytorch, where elements might not end up as part of the computation graph.

A GFNModule packages all together the elements required to train a gflownet with a particular loss. It contains the parameters but a lot of other logic too, which is good to co-locate.

I fear you are focusing on a low-priority optimization at this stage, but maybe I don't really understand your proposal, I'm open to clarification, but I currently doubt we should be focused on this particular issue right now.

[hyeok9855]

Okay, let's leave it as is for now. Maybe revisit this issue later if necessary.