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
|
lkdvos
left a comment
There was a problem hiding this comment.
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.