Skip to content
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

AutoregressiveBisectionInverter bounds picked up as trainable parameters #173

Closed
mdmould opened this issue Sep 11, 2024 · 1 comment · Fixed by #174
Closed

AutoregressiveBisectionInverter bounds picked up as trainable parameters #173

mdmould opened this issue Sep 11, 2024 · 1 comment · Fixed by #174

Comments

@mdmould
Copy link
Contributor

mdmould commented Sep 11, 2024

Because the lower and upper bounds for the inverter are arrays, they get picked up as trainable parameters when filtering with, e.g., is_inexact_array. I think this isn't desired behaviour in any cases, because it would probably interact unexpectedly with the adaptive bounding in the bisection search. Usually this won't affect anything because a flow that contains the inverter, e.g., BNAF, is trained only without ever using the numerical inverse. The only reason I noticed this was because I was counting the number of parameters I expected in the model.

I'm not sure if this is really an issue in practice. In any case, one can just wrap the inverter in a non_trainable or manually ignore the lower and upper "parameters" as necessary. But maybe there's a neater solution that keeps the bisection functions compatible with the scans and so on?

@danielward27
Copy link
Owner

Good spot, thanks. Your absolutely right that they shouldn't be included, I'll get that fixed. Like you said generally it shouldn't matter (I believe JAX will error if you try to differentiate through the numerical method), but one case where I believe it matters is if regularisation of parameters is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants