Skip to content

Conversation

lkdvos
Copy link
Contributor

@lkdvos lkdvos commented Apr 3, 2025

This is a refactor of the Sector type to be:
a) an AbstractUnitRange as a first step towards GradedArraysNext
b) wrapping a TensorKitSectors.Sector to make use of the sector data that is defined there.

@ITensor ITensor deleted a comment from mtfishman Sep 25, 2025
@ITensor ITensor deleted a comment from mtfishman Sep 25, 2025
@ITensor ITensor deleted a comment from mtfishman Sep 25, 2025
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 93.00000% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.54%. Comparing base (a0e5ade) to head (965dd7c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/sector_product.jl 93.71% 11 Missing ⚠️
src/abstractsector.jl 91.89% 9 Missing ⚠️
ext/GradedArraysSUNRepresentationsExt.jl 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   94.18%   93.54%   -0.64%     
==========================================
  Files          20       12       -8     
  Lines         997      914      -83     
==========================================
- Hits          939      855      -84     
- Misses         58       59       +1     
Flag Coverage Δ
docs 0.00% <0.00%> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lkdvos lkdvos changed the title [WIP] TensorKitSectors Sectors refactor Sep 25, 2025
@lkdvos lkdvos marked this pull request as ready for review September 25, 2025 15:29
@lkdvos lkdvos force-pushed the sectors branch 3 times, most recently from abc78e4 to fcde4f7 Compare September 25, 2025 16:23
@lkdvos
Copy link
Contributor Author

lkdvos commented Sep 29, 2025

I now went through and incorporated the changes we discussed last week. From my local testing, everything but the show tests are passing right now, but I noticed some inconsistencies in these methods so I wanted to first check in and discuss before I make big changes.

Let me first discuss the rest of the PR: We now have SectorRange, which is a light wrapper around a TensorKitSectors.Sector type that subtypes AbstractUnitRange{Int}. This acts as a OneTo(quantum_dimension(sector)), with some additional functionality.

As a result, most of the sector definitions could be removed and simply inherited from the definitions of the data in TensorKitSectors. There are however two exceptions/special cases.
The first being SU(N), where I can in principle write a package extension for SUNRepresentations to immediately inherit the implementation for all N, with the unfortunate restriction that I can't really define a type alias for this as in the other cases, since that would then live in a package extension and I can't export it from there.
The other being the ProductSector, which I kept here as a subtype of TensorKitSectors.Sector but needs a little bit more work about the additional data (clebsch-gordan, Rsymbol, Fsymbol, ...) to fully adhere to the TensorKitSectors interface. However, since that was data that was already missing I'm not adding it here, and I'm happy to revisit this when we start needing it.

There are some slightly awkward parts of this design, mostly related to the different structs at the different levels. With this PR, a gradedrange has the following hierarchy:
GradedUnitRange <- SectorUnitRange <- SectorRange <- TensorKitSectors.Sector
From right to left, we add first the range supertype, then an isdual and multiplicity, and finally combine multiple SectorUnitRanges into a single GradedUnitRange.
I think this will be better once we replace SectorUnitRange with the actual CartesianProduct of a unitrange and a sectorrange, which also means that we indeed have to add an isdual flag to the SectorRange type, which I would like to leave for a follow-up PR.

For the show methods, I think there were some inconsistencies previously with when to output machine or human parse-able results. I think in general the accepted standard (which is not enforced and badly documented) is to use Base.show(::IO, x) for a result that can be parsed, ie eval(Meta.parse(sprint(show, x))) == x, while the 3-arg Base.show(::IO, ::MIME, x) can be human-readable. There are some subtle design choices to consider that have to do with namespaces, where in principle you could decide to output GradedArrays.U1(1) or U1(1) when U1 is not explicitly available in the namespace. I'm mostly fine with any decision here, but I would like it to be consistent across the board

@lkdvos lkdvos requested a review from mtfishman October 8, 2025 17:18
Copy link
Contributor Author

@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.

Seems like the new github may have deleted some of my previous comments, so there might be duplicates now

@mtfishman
Copy link
Member

mtfishman commented Oct 9, 2025

I can't seem to reply inline to some of your comments, so I'm responding here.

You asked about defining const × = sectorproduct vs. defining them separately, I guess I'm fine either way. We could start with const × = sectorproduct and then if their interfaces diverge we could separate them, but I assume their interface should be the same anyway.

Regarding SectorProductRange, I don't mind too much since as you say it should be internal so we can keep it as-is.

@lkdvos
Copy link
Contributor Author

lkdvos commented Oct 9, 2025

Unless I'm missing something, this should have addressed all the comments

@mtfishman
Copy link
Member

Thanks for working through all of this! It will be nice to be able to easily use the fancier symmetries being developed based on TensorKitSectors.

@mtfishman mtfishman merged commit 7c03f1a into main Oct 9, 2025
15 of 18 checks passed
@mtfishman mtfishman deleted the sectors branch October 9, 2025 22:59
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