-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix combining interfaces #29
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
- Coverage 72.89% 72.44% -0.46%
==========================================
Files 9 9
Lines 321 323 +2
==========================================
Hits 234 234
- Misses 87 89 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Yeah, it would be nice to have:
|
Gotcha, I'll try it out. Partially I was pinging you to make sure this is orthogonal to what you're working on in #28, which I think it is. |
@lkdvos I decided to make this PR more incremental, since I think more ambitious plans would require some breaking changes and also lead to conflicts with #28, so we are better off merging #28 first and then looking into more ambitious changes to this part of the code after that. But, this PR is still helpful since it fixes a bug I was hitting when broadcasting NamedDimsArrays wrapping BlockSparseArrays (see ITensor/NamedDimsArrays.jl#47), and also makes the code a bit clearer. Basically, the plan in a future PR would be to bring this more in line with the design of
|
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.
I think when I was looking at it I had a similar feeling, that the Base approach was definitely reasonable. What you are describing sounds great, and since it aligns with Base that also removes the mental bandwidth of having to remember two different systems so I'm definitely in favor of this.
Changes here also look good, it's convenient that it's not breaking, and should be easy to rebase the other PR on top of.
This fixes combining interfaces with more than two objects.
@lkdvos at some point I think you brought up making the design of this system closer to the design of BroadcastStyle (i.e. the interface for defining and combining interfaces), maybe I'll try that out here as well.