-
Notifications
You must be signed in to change notification settings - Fork 81
Add WeightingInterface link to SolverInterface #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @GiovanniCanali! I was having a look at this even tho it is not marked as review yet. I like your changes! One thing I would try is to add a kwarg (e.g. update_every_nepoch or something similar) to all methodologies where the values of the weights are updated (ofc this doesn't change the "static" weightings). A simple way to achieve this is to put it in the init of weighting interface. Then we can make a method in the same class (e.g. update_and_aggregate or something similar) which we simply overload aggregate and call it if trainer.current_epoch% update_every_nepoch==0. This method is also called in the solver in _optimization_cycle instead of the current aggregate. What do you think? |
I agree with you. I just need to think a bit about the changes with respect to the solver, but it should not be a big deal. |
I think we just need to change this line https://github.com/mathLab/PINA/blob/master/pina/solver/solver.py#L252 with the new update method which overloads aggregate |
0ddc901
to
ca8f370
Compare
dd83440
to
713b169
Compare
@GiovanniCanali For me is ready, let me know what you think about it! |
4a08069
to
ab22331
Compare
@GiovanniCanali I see some tests are failing. This is because some solvers override _optimization_cycle. That's not something it supposed to happen. Maybe we can modify those bugs now, merge the PR in dev and update those solvers such that _optimization_cycle is only in solverinterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great @dario-coscia, everything looks good to me.
I just highlighted some minor issues related to the documentation, let's fix them before merging!
bc81ad0
to
d7909b1
Compare
Hi @dario-coscia, This PR is now complete and ready for review. Commits have been squashed, so that we now have one commit per new feature. Tests are failing due to #621. I would suggest fixing that issue before merging, so we can also check that the tests pass on Ubuntu and macOS. |
d7909b1
to
549bc7e
Compare
Co-authored-by: Dario Coscia <[email protected]>
549bc7e
to
9e4b238
Compare
Description
This PR fixes #619.
This PR also adds the
SelfAdaptiveWeighting
class. Reference (page 9): https://arxiv.org/pdf/2507.08972.Moreover, it introduces the possibility to update weights via the
update_and_aggregate
and_update
methods.Checklist