fix: transform_to_unconstrained device mdn device handling (#1893)#1896
Conversation
- Move mcmc_transform tensor internals to target device after construction (bounded branch via biject_to was CPU-only) - Override _apply in MixtureDensityEstimator so _prior_transform follows model.to(), .cuda(), .mps() device moves - Add _move_transform_tensors_to_device helper that recursively walks the transform tree (ComposeTransform, IndependentTransform, etc.)
505bb34 to
7e2df57
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1896 +/- ##
==========================================
- Coverage 87.88% 87.82% -0.07%
==========================================
Files 143 143
Lines 13387 13474 +87
==========================================
+ Hits 11765 11833 +68
- Misses 1622 1641 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@janfb please review when you have time thanks! and anything needed |
janfb
left a comment
There was a problem hiding this comment.
Thanks @BHARATH0153 , looks good! It's focused on MDN for now and it works nicely. I just suggest a bit of refactoring towards a more general _move_tensor function.
The zuko and state-dict problems are not fixed here, which is fine. We can do it in a follow up.
|
One more thing: could you add test coverage for this? Codecov has it at ~20% atm. The test should be @pytest.mark.gpu (it'll auto-skip locally and run on the GPU job, like the other tests in tests/inference_on_device_test.py): build an MDN with z_score_x="transform_to_unconstrained", est.to("cuda"), then assert log_prob runs and the transform's tensors are on CUDA or MPS. |
…sform Replace _move_transform_tensors_to_device with generic _apply_to_transform that accepts a callable fn, not just a device string. Drop unused bare-tensor branch from list/tuple walker (transforms only hold sub-transforms in lists, never tensors). Port MixtureDensityEstimator._move_tensors callers to the shared helper and remove the now-duplicate static method. Add @pytest.mark.gpu test verifying MDN transform tensors move with .to() and that log_prob / sample run on the target device.
c98155e to
3d0e223
Compare
|
@janfb once look into this please |
janfb
left a comment
There was a problem hiding this comment.
Thanks @BHARATH0153 , this is exactly the refactor I had in mind, the core code looks good now. Now there are only things on the new tests that need to be fixed, and one small cleanup, then we can merge soon.
8d73994 to
4a1e306
Compare
|
@janfb thanks! for the review done please check. |
janfb
left a comment
There was a problem hiding this comment.
Looks good, well done @BHARATH0153 ! 👏
Fixes MDN part of #1893
Two changes:
mcmc_transformbounded branch now moves transform tensor internals to the requesteddevice(was CPU-only viabiject_to)MixtureDensityEstimator._applyoverridden so_prior_transformfollows.to(),.cuda(),.mps()moves