Repository navigation
Fix joint full-batch and minibatch condition loading - #863
Conversation
GiovanniCanali
added
bug
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
49ad884 to
e1c988b
Compare
Refactor data module to improve batching and aggregation methods. Update batching mode and implement condition row aggregation.
GiovanniCanali
left a comment
GiovanniCanali
left a comment
There was a problem hiding this comment.
Looks good, @ndem0!
Please, remove the print statements and commented code.
| ) | ||
|
|
||
| print("Condition losses:", condition_losses) | ||
|
|
There was a problem hiding this comment.
Remove print statement.
| for batch_id in indices[val_end:]: | ||
| test_rows.append([condition_id, batch_id]) | ||
| # test_rows.append([condition_id, batch_id]) | ||
| condition_test_rows.append([condition_id, batch_id]) |
There was a problem hiding this comment.
Remove commented code.
|
|
||
| print("Train rows:", train_rows) | ||
| print("Val rows:", val_rows) | ||
| print("Test rows:", test_rows) |
There was a problem hiding this comment.
Remove print statements.
dario-coscia
left a comment
dario-coscia
left a comment
There was a problem hiding this comment.
I am not a fan of [B, conditions, 2]; maybe I prefer tuple([B, conditions]) idk if it is feasible and equivalently faster. Or even better iterate on a NamedTuple data, condition_name. What do you think?
Not really sure I got what you meant. We're still wondering if a |
|
I'm going to merge this PR even though modifications are still needed, I'm afraid to keep this small patch open for too long. We are still in the |
Description
This PR fixes #862
Restore joint-condition optimizer steps following the condition/datamodule refactor. Previously, the refactored loader yielded one condition batch per step, changing the training objective between steps and causing instability, especially with L-BFGS.
The datamodule now:
Additional tests are generated by an AI agent.
Checklist