Closed mdmould closed 1 year ago
Hey @mdmould (again),
Good idea. Left some comments on the implementation.
Cheers,
Artur
It's not clear to me that this change won't make some models fit worse than they did before. I'm not convinced it can be changed blindly (after all, if it made no difference, why bother?).
It's not clear to me that this change won't make some models fit worse than they did before. I'm not convinced it can be changed blindly (after all, if it made no difference, why bother?).
@imurray This is a non-trainable transformation with fixed parameters (note both shift
and scale
are buffers), so this will have no impact on model fits. The only effect is that you can now apply e.g. PointwiseAffineTransform(shift=0, scale=-1)
(y = -x).
I renamed to _log_abs_scale
and added test with negative scales.
Cheers, Matt
I also added a test to check that scale=0 is caught.
Thank you for taking the time, @mdmould, and sorry for the delay: merged!
The current implementation of PointwiseAffineTransform allows only positive values for the scale. As far as I can tell, this isn't necessary and only zero values (which make the transform no longer bijective) need to be rejected. Only the absolute value of the scale enters the log prob conversion while the scale and its reciprocal itself enters the transform of samples.