-
Notifications
You must be signed in to change notification settings - Fork 128
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
Refactor nsf #537
Refactor nsf #537
Conversation
2b9a0fc
to
5eabd5f
Compare
Codecov Report
@@ Coverage Diff @@
## main #537 +/- ##
==========================================
- Coverage 66.80% 66.77% -0.03%
==========================================
Files 67 67
Lines 4187 4199 +12
==========================================
+ Hits 2797 2804 +7
- Misses 1390 1395 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Thanks for the refactor!
Regarding the warning: I think you're right, stacking multiple rational-quadratics will be non-rational-quadratic, so it might be more flexible. Out of interest: do you have empirical evidence that it improves performance?
thanks for the review @michaeldeistler and sorry, I extended the refactor just now. Regarding empirical evidence, yes! when optimising the MNLE architectures we found that more transforms performed much better. Of course they were slower too, so we found a good trade-off of two transforms in the end. |
Ah, interesting, thanks! Feel free to merge once tests are passing. |
- refactor building of transform into lists - allow multiple nfs transforms for 1d x. - refactor spline context class - add tail_bound and hidden_layers kwargs. - remove nsf transforms warning for 1D x.
refactoring the usage of
nsf
. Importantly, in case of a one-dimensionalx
we are currently warning that in this case it wouldn't help to have more than one transform.However, I think it still makes sense to have multiple transforms. In this case the spline parameters are learned from the conditioning variables only, because there is no "other" dimension in
x
to learn from. But we can still use multiple transforms to stack conditioners and get more flexibility.Maybe I am missing something here? But in my current view this makes sense and we should remove the warning.