alexhernandezgarcia / gflownet

Generative Flow Networks - GFlowNet
https://gflownet.readthedocs.io/en/latest/
Apache License 2.0
167 stars 11 forks source link

[Tiny] Fix scrabble test #289

Closed alexhernandezgarcia closed 7 months ago

alexhernandezgarcia commented 7 months ago

One of the Scrabble tests was disabled in a previous PR because it was failing. This PR fixes it. The reason for the fail was that the state had to be copied because the method step() of the Scrabble env does not copy the state unlike other envs.

josephdviviano commented 7 months ago

LGTM - can you add a comment explaining why copy is required here?

alexhernandezgarcia commented 7 months ago

@josephdviviano copy is needed because of the following:

Does that make sense?

josephdviviano commented 7 months ago

@alexhernandezgarcia it does make sense indeed - I just meant a comment in the test code so people in the future can also understand why there need be a copy there.

alexhernandezgarcia commented 7 months ago

@alexhernandezgarcia it does make sense indeed - I just meant a comment in the test code so people in the future can also understand why there need be a copy there.

Oh, but there are multiple places where copy is used and the explanation is kind of long :/ Should I add a general comment at the beginning of the script?

alexhernandezgarcia commented 7 months ago

Comment about copies added here.

josephdviviano commented 7 months ago

I added some inline comments as well. Thanks!