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 last label #6

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Fix last label #6

merged 2 commits into from
Dec 17, 2024

Conversation

ogauthe
Copy link
Contributor

@ogauthe ogauthe commented Dec 16, 2024

This PR fixes a lost label in last. The lost label triggered a type unstability which is now fixed. However, I have not been able to add a test for this case. No function allows to create an AbstractUnitRange with empty lasts. It can still be initialized explicitly, but it has many issues (first crashes), preventing testing.

Maybe we should just return last(a.lasts) and accept a crash for empty a.lasts (which is still the current behavior)?

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@4f628c4). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main       #6   +/-   ##
=======================================
  Coverage        ?   76.15%           
=======================================
  Files           ?        8           
  Lines           ?      369           
  Branches        ?        0           
=======================================
  Hits            ?      281           
  Misses          ?       88           
  Partials        ?        0           
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.

@mtfishman
Copy link
Member

mtfishman commented Dec 16, 2024

However, I have not been able to add a test for this case. No function allows to create an AbstractUnitRange with empty lasts. It can still be initialized explicitly, but it has many issues (first crashes), preventing testing.

Here is one way to construct it:

julia> r = gradedrange(Pair{String,Int}[])
GradedUnitRanges.GradedOneTo{LabelledNumbers.LabelledInteger{Int64, String}, Vector{LabelledNumbers.LabelledInteger{Int64, String}}}

but as you pointed out I see that first(r) crashes. I think it isn't well defined since we haven't specified the label (it should output a labelled 1 value following the convention of BlockArrays.AbstractBlockedUnitRange), maybe we should support:

julia> r = gradedrange("x" => 1, Pair{String,Int}[])

julia> r = gradedrange("x", Pair{String,Int}[]) # If only the label is specified, assume the value starts at 1

to allow specifying the first value/label (not saying we need to do that in this PR, I'm just trying to put this issue into a bigger context).

Maybe we should just return last(a.lasts) and accept a crash for empty a.lasts (which is still the current behavior)?

The current code matches the behavior of last(::AbstractBlockedUnitRange):

julia> using BlockArrays

julia> first(blockedrange(1, Int[]))
1

julia> last(blockedrange(1, Int[]))
0

julia> first(blockedrange(Int[]))
1

julia> last(blockedrange(Int[]))
0

which follows from Julia's definition for empty ranges:

julia> first(1:0)
1

julia> last(1:0)
0

so I think it makes sense as it is, with your new improvements.

@mtfishman mtfishman changed the title fix last label Fix last label Dec 16, 2024
@ogauthe
Copy link
Contributor Author

ogauthe commented Dec 17, 2024

We could indeed allow for r = gradedrange("x", Pair{String,Int}[]) , although I would wait for a use case before implementing it. So I am fine with merging the code as it is, I just bumped the version according to #5

@mtfishman mtfishman merged commit 59e2fe6 into ITensor:main Dec 17, 2024
11 checks passed
@ogauthe ogauthe deleted the label_last branch December 17, 2024 13:29
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