Skip to content
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

Merged
merged 3 commits into from
Feb 28, 2025
Merged

Fix combining interfaces #29

merged 3 commits into from
Feb 28, 2025

Conversation

mtfishman
Copy link
Member

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.

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.

Project coverage is 72.44%. Comparing base (8371a5e) to head (3ce69a4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/abstractinterface.jl 40.00% 6 Missing ⚠️
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     
Flag Coverage Δ
docs 37.18% <9.09%> (-0.55%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lkdvos
Copy link

lkdvos commented Feb 27, 2025

Yeah, it would be nice to have:

  • symmetric promotion rules where you only need to define one
  • a clear entry point, what do we need to overload?
  • a way to signal that no promotion could be found

@mtfishman
Copy link
Member Author

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.

@mtfishman mtfishman changed the title [WIP] Fix combining interfaces Fix combining interfaces Feb 28, 2025
@mtfishman mtfishman marked this pull request as ready for review February 28, 2025 02:13
@mtfishman mtfishman requested a review from lkdvos February 28, 2025 02:19
@mtfishman
Copy link
Member Author

mtfishman commented Feb 28, 2025

@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 Base.Broadcast.BroadcastStyle, which would mean that:

  1. types would overload DerivableInterfaces.AbstractInterface(::Type) to define their interface as opposed to the current DerivableInterfaces.interface(::Type) syntax,
  2. interface combining rules would be defined through overloading DerivableInterfaces.AbstractInterface(::AbstractInterface, ::AbstractInterface) as opposed to the current DerivableInterfaces.combine_interface_rule(::AbstractInterface, ::AbstractInterface) syntax, and
  3. combine_interfaces(x...) would be used to get the combined interface of a number of objects/arguments, as opposed to the current interface(x...) syntax.

Base.Broadcast seems to have made some reasonable choices so it seems like we may as well just follow what they did for this and other similar trait systems. Curious what you think.

Copy link

@lkdvos lkdvos left a 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.

@mtfishman mtfishman merged commit 5aa5aff into main Feb 28, 2025
15 of 17 checks passed
@mtfishman mtfishman deleted the fix_combine_interfaces branch February 28, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants