Repository navigation
Fix Stabilize Stiefel init and match PyTorch’s generator API - #43
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bruAristimunha
left a comment
There was a problem hiding this comment.
Thanks @lucas-heck! This is a good cleanup. Dropping the hidden "seed 0 when no seed is given" default and following torch.manual_seed / generator like torch.nn.init is the right call. The batched torch.linalg.qr(..., mode="reduced") fallback also removes the per-matrix loop.
Checked before merging:
- Full suite with all four open PRs merged together: green.
- Apple MPS (the float64-on-CPU path): float32
16×16batched Stiefel init givesmax|WᵀW − I| ≈ 1.8e-7, and the result comes back onmpsin float32. - No remaining
seed=callers ofBiMap/stiefel_inspd_learn/, the examples, or the docs.
One note for the release, not blocking: removing seed from BiMap / stiefel_ breaks existing calls, which will now raise a TypeError. For a 0.x release, the entry under "Changed" is enough. I'll mention it in the release notes.
I'll merge main into the branch to resolve the CHANGELOG.md conflict (both entries kept), then merge. Thanks!
# Conflicts: # CHANGELOG.md
Problem
Stiefel initialization can produce weights that fail orthogonality checks in float32, particularly for square matrices.
It also used a seed argument, unlike the other initialization methods and PyTorch's convention of accepting an optional generator.
Fix
stiefel_'sseedargument withgeneratorand removeBiMap'sseedargument. Reproducible BiMap initialization now followstorch.manual_seed.Verification
1e-5, added the previous failing CPU float32 case, and enabled the square Stiefel BiMap case.1e-5.