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 an issue with unsorted deleteat #186

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Fix an issue with unsorted deleteat #186

merged 2 commits into from
Oct 14, 2024

Conversation

lkdvos
Copy link
Collaborator

@lkdvos lkdvos commented Sep 3, 2024

There was a small issue when the trace indices were generated in unsorted order, which made deleteat complain. This should resolve that.

@Jutho
Copy link
Owner

Jutho commented Sep 29, 2024

I missed this PR. Do you happen to remember an example that triggered this? Might be good to add to the tests.

@lkdvos
Copy link
Collaborator Author

lkdvos commented Sep 29, 2024

I think all you need is more than one pair of trace indices that are not sorted. It's only used in the cutensor implementation, so it might not have run on CI because of that, although I'm not so sure that we have something that triggers it.

@Jutho
Copy link
Owner

Jutho commented Sep 30, 2024

I am actually confused why deleat complains. It sorts the indices internally exactly in order not to run into such problems:
https://github.com/Jutho/TupleTools.jl/blob/2a6b88c8153a0a0d83ec3cde60b9937d9122402b/src/TupleTools.jl#L119

@Jutho
Copy link
Owner

Jutho commented Oct 4, 2024

Should this be part of the 5.0.2 release? I am not opposed to it, but if this fix is necessary, it points to a bug in TupleTools that I would like to understand.

@lkdvos
Copy link
Collaborator Author

lkdvos commented Oct 4, 2024

I'm fine with having this separate if necessary, could be 5.0.3, I should have access to my laptop again starting next week so I can check then to make sure I wasn't just doing something silly.

(I should apparently read my own advice more carefully: including a MWE, a Stacktrace etc, would have resolved this issue already...)


descA = CuTensorDescriptor(A; size=szA, strides=stA′)
descC = CuTensorDescriptor(C)

modeA = collect(Cint, deleteat!(Ainds, q[2]))
modeA = collect(Cint, deleteat!(Ainds, qsorted))
Copy link
Collaborator Author

@lkdvos lkdvos Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked, actually it is here that the issue stems from: Ainds is a vector, rather than a tuple, so this does not go via TupleTools. I will try the following:

  • make sure trace_indices returns sorted indices
  • add a test for this

Edit: I cannot fix this in trace_indices, and was too quick in suggesting that. I do think the current solution is the easiest, but will still try and add a test.

@lkdvos lkdvos requested a review from Jutho October 7, 2024 11:07
@lkdvos lkdvos merged commit 3433bc9 into master Oct 14, 2024
15 checks passed
@lkdvos lkdvos deleted the ld-planarfix branch October 14, 2024 11:20
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