Refactor Detailed Balance #432
Conversation
younik
left a comment
younik
left a comment
There was a problem hiding this comment.
good, thank you @hyeok9855 !
src/gfn/gflownet/detailed_balance.py
Outdated
| scores = preds - targets | ||
| # Apply forward-looking if applicable | ||
| if self.forward_looking: | ||
| import warnings # type: ignore |
There was a problem hiding this comment.
logging should have typing
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
im not sure I understand either. but why is type: ignore required for an import?
There was a problem hiding this comment.
I thought it was required to avoid a Python error, but it turned out to be okay without it. Fixed.
josephdviviano
left a comment
josephdviviano
left a comment
There was a problem hiding this comment.
lgtm! but in the future I think it would be easier for me if you describe in your PR the intention behind the change (e.g., why you are doing a particular refactor) - it's hard for me to see why this was a priority for you, and I think knowing that would help me catch more subtle aspects of the changes.
| flow of the states. | ||
| forward_looking: Whether to use the forward-looking GFN loss. | ||
| log_reward_clip_min: If finite, clips log rewards to this value. | ||
| safe_log_prob_min: If True, uses -1e10 as the minimum log probability value |
There was a problem hiding this comment.
why did you remove this? it's useful.
There was a problem hiding this comment.
log_reward_clip_minis in thePFBasedGFlowNetnowsafe_log_prob_minwas not used anywhere.
src/gfn/gflownet/detailed_balance.py
Outdated
| scores = preds - targets | ||
| # Apply forward-looking if applicable | ||
| if self.forward_looking: | ||
| import warnings # type: ignore |
There was a problem hiding this comment.
im not sure I understand either. but why is type: ignore required for an import?
| ) -> None: | ||
| """Initializes a TrajectoryBasedGFlowNet instance. | ||
| Args: |
There was a problem hiding this comment.
I'm a bit confused about how you were able to get rid of this, but seems fine!
There was a problem hiding this comment.
It calls super().__init__(...) and does nothing else, so I could remove it.
|
That's funny, we needed the safe log prob min to solve a much earlier
issue. I suppose it was removed by someone. (see reply - i was wrong)
Joseph (Mobile)
…On Wed, Nov 19, 2025 at 10:51 Sanghyeok Choi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/gfn/gflownet/detailed_balance.py
<#432 (comment)>:
> @@ -60,11 +60,9 @@ class DBGFlowNet(PFBasedGFlowNet[Transitions]):
logF: A ScalarEstimator or ConditionalScalarEstimator for estimating the log
flow of the states.
forward_looking: Whether to use the forward-looking GFN loss.
- log_reward_clip_min: If finite, clips log rewards to this value.
- safe_log_prob_min: If True, uses -1e10 as the minimum log probability value
- log_reward_clip_min is in the PFBasedGFlowNet now
- safe_log_prob_min was not used anywhere.
—
Reply to this email directly, view it on GitHub
<#432 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7TL2U3TD4DHOB2EE2CGXL35SGW3AVCNFSM6AAAAACLVXASXWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTIOBTGQ4TQNBYGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Actually I just looked through the logs and it looks like it was never used anywhere. I guess I forgot to wire it in or something. I wonder why I did that?? |
@josephdviviano The intention behind these is... I just saw some suboptimal designs in the repo and wanted to improve them. |
|
Great work! Thanks for catching that stuff!
Joseph (Mobile)
…On Wed, Nov 19, 2025 at 11:02 Sanghyeok Choi ***@***.***> wrote:
*hyeok9855* left a comment (GFNOrg/torchgfn#432)
<#432 (comment)>
lgtm! but in the future I think it would be easier for me if you describe
in your PR the intention behind the change (e.g., why you are doing a
particular refactor) - it's hard for me to see why this was a priority for
you, and I think knowing that would help me catch more subtle aspects of
the changes.
@josephdviviano <https://github.com/josephdviviano>
The work in this PR was originally a part of #431
<#431>, but I separated it so as
not to make each PR too big.
The intention behind these is... I just saw some suboptimal designs in the
repo and wanted to correct them.
—
Reply to this email directly, view it on GitHub
<#432 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7TL2TFABD646JDRWHKFDD35SIBXAVCNFSM6AAAAACLVXASXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKNJTGUYDAMBWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Description
The results from
python tutorials/examples/train_hypergrid_simple.py --loss DBare identical to themasterbranch.