Closed josephdviviano closed 11 months ago
Hey guys, I'm sorry for the delay in responding here, it's been a busy week at work with a lot of GitHub notifications 😅
I don't think this PR is quite right. PFBasedGFlowNet
shouldn't need to redefine the Generic[TrainingSampleType]
as a subclass because it is already exactly subclassed by GFlowNet
. Further, I think that TrajectoryBasedGFlowNet(PFBasedGFlowNet[Trajectories])
is the right signature for TrajectoryBasedGFlowNet
because it is a subclass of PFBasedGFlowNet
which uses Trajectories
as the type returned from to_training_samples
.
I've tried to revert this commit locally and reproduce the error but I cannot. Are there more concrete reproduction steps for that? I'd be happy to take a crack at it.
If you guys hang out in a Slack or Discord, I'd be happy to chat further / make myself available for easier @-mentioning.
Thanks!
No other steps. If you are on master and using the exact environment in the pyproject.toml, the tests will fail. This was also reproduced by some of our collaborators at Intel.
Could you send your environment? We can diff with mine to see if there's anything there. It could be the python version behaviour changing slightly.
Thanks for offering your help. I don't really understand this typing scheme yet.
Let me see if I can get you into the Mila slack as a guest! Joseph Viviano @josephdviviano https://twitter.com/josephdviviano viviano.ca
On Fri, Oct 6, 2023 at 5:37 AM Mike Arpaia @.***> wrote:
If you guys hang out in a Slack or Discord, I'd be happy to chat further / make myself available for easier @-mentioning.
— Reply to this email directly, view it on GitHub https://github.com/saleml/torchgfn/pull/139#issuecomment-1750291180, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7TL2UJOXSP5EOGFJHJ6RLX57GO5AVCNFSM6AAAAAA5TKRPE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJQGI4TCMJYGA . You are receiving this because you were assigned.Message ID: @.***>
@marpaia can you send me your email so I can add you to the mila slack
Awesome, thanks @josephdviviano, I've just sent you an email!
This adds
Generic
toPFBasedGFlowNet
andTrajectoryBasedGFlowNet
, previously these imports were broken.Note I am not 100% sure that
TrainingSampleType
is correct for both of these classes, I don't fully understand how to useGeneric
properly.