-
Notifications
You must be signed in to change notification settings - Fork 59
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
Flexible Default Settings Across Inference Networks #296
Conversation
I think this proposal looks sensible to me. We should definitely (a) have very good defaults that (b) can be selectively updated by the user while retaining all other default settings. I think your proposal achieves both in a very simple manner. |
This is ready to merge upon your review @LarsKue / @paul-buerkner. |
@LarsKue Interesting, the cycle consistency test seems to be failing for flow matching, even though it's fitting a couple of benchmarks...The reason seems to be a numerical one. The output values are close, but not within the tolerance threshold. I suspect the reason is that the defaults are now creating a significantly larger network. Can we increase the precision of the cycle consistency test for FM? |
I cannot judge the details of the defaults themselves (for this I trust you, @stefanradev93, as the neural network whisperer). But the general structure of updating default configs looks good to me. I approve but will wait for @LarsKue to check the test. |
The default for Flow Matching is now adaptive step size, which can mess with cycle consistency. I am already looking into improved test parametrization, which could allow us to circumvent this issue by using fixed step size or increasing the tolerance for adaptive step size only. |
An even simpler solution would be to use a tiny network in the tests. |
I don't think this necessarily solves the issue with the cycle consistency, although it is of course desirable with respect to speeding the tests up. The problem is that since the step size is adaptive, we necessarily end up evaluating any curved velocity field at different time points when integrating forward than backward. |
I would also like to consider using a general |
Yes, the factory would be nice to have, and will also solve the get_/from_config shuffling. For now, let's use fixed step size / smaller networks for the tests or relax the cycle consistency constraints for FM. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
|
Adding this WIP in anticipation of the integrators PR to discuss dynamically setting default settings for subnets across different inference networks. The current straightforward proposal makes use of a default class
dict
for each inference net like:The default config is only considered if the default subnet is an
mlp
and not a user-provided backbone. In the former case, the fields of the default config are updated with user-provided fields forsubnet_kwargs
(if any) and the inference network is spawned. A working draft can be inspected, for instance, infree_form_flow
. Let me know what you think @paul-buerkner @LarsKue