-
Notifications
You must be signed in to change notification settings - Fork 0
Fix corner cases in reshape and improve its robustness #72
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
Conversation
gausshj
commented
Oct 24, 2025
- Fix corner cases for reshape none edge into (1, 1, ...) shape
- Add input new shape validity check
- Add corner cases in reshape
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| merging_sign: list[tuple[int, torch.Tensor]] = [] | ||
|
|
||
| original_self_is_scalar = self.tensor.dim() == 0 | ||
| if original_self_is_scalar: |
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.
把这个判断放最外面挺好的,不过为啥只有original是scalar的判断,没有target是scalar的判断?
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.
放在外面确实好很多,现在已经在607dfd8把target判断放在了外面。代码复杂度和可读性大大提高了。
| return even, odd | ||
|
|
||
| @staticmethod | ||
| def calculate_even_odd(edges: tuple[tuple[int, int], ...]) -> tuple[int, int]: |
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.
这两个函数有一个就行,不用封装得这么狠,再说,你这个函数也没用到上面那个函数。
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.
好的,已经在607dfd8中提交了修改。
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.
这个函数原则上可以被上面那个reorder_indices覆盖功能, 但是你在结果是(1, 0)的情况下, 希望做简单/快速的判断. 所以我建议: 直接写一个 is_xxx_valid 之类的函数, 或者直接判断 edges_only.count((0, 1)) % 2 == 0 .
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.
这里这个函数后面开发会不会也有用?我在svd的测试中也用到了类似的函数,这里实现了后续可以直接调用。
grassmann_tensor/tensor.py
Outdated
| def calculate_even_odd(edges: tuple[tuple[int, int], ...]) -> tuple[int, int]: | ||
| even, odd = 1, 0 | ||
| for e, o in edges: | ||
| even, odd = even * e + odd * o, even * o + odd * e |
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.
试试用functool reduce写吧,其他地方是用reduce写的,保持下一致性
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.
好的,已经在607dfd8中提交了修改。
hzhangxyz
left a comment
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.
内容都没啥问题, 只是看着代码习惯不大好, 比如eo你过个一个月就认不出来是even odd的意思了...
grassmann_tensor/tensor.py
Outdated
| f"Ambiguous integer dim {item} from scalar. " | ||
| "Use explicit (even, odd) pairs, or only use 1 for trivial edges." | ||
| ) | ||
| normalized.append((1, 0)) |
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.
normalized 这个变量的名字是怎么来到? 这个的含义是 归一化的, 你这里是new_shape_list ?
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.
部分参考了GPT,这里是因为输入new_shape是tuple[int | tuple[int, int], ...],但是在“标量情况”中(self.tensor.dim() == 0),不允许这种混合,normarized是类型规范化的意思。
| return even, odd | ||
|
|
||
| @staticmethod | ||
| def calculate_even_odd(edges: tuple[tuple[int, int], ...]) -> tuple[int, int]: |
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.
这个函数原则上可以被上面那个reorder_indices覆盖功能, 但是你在结果是(1, 0)的情况下, 希望做简单/快速的判断. 所以我建议: 直接写一个 is_xxx_valid 之类的函数, 或者直接判断 edges_only.count((0, 1)) % 2 == 0 .
那我要不加上注释或者把变量名改成易读懂的类型? |
- Fix corner cases when reshaping a None edge into (1,1,…) while preserving arrow/parity. - Validate and normalize new_shape; reject incompatible dims early. - Move `target` check outside the while-loop to reduce nesting and improve readability. - Unify `_calculate_even_odd` and `calculate_even_odd` via `functools.reduce`. - Add/extend corner-case tests for reshape to prevent regressions.
27e75dc to
f4979fa
Compare