Skip to content

Minimal reproducer for breakage introduced by PR #3402. #3478

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

Closed
wants to merge 2 commits into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Nov 18, 2021

Description

WIP — Starting with a minimal reproducer.

Breakage introduced by PR #3402 (comment)

Suggested changelog entry:

@rwgk rwgk requested a review from jagerman November 18, 2021 04:15
@henryiii
Copy link
Collaborator

henryiii commented Nov 18, 2021

@jagerman, it looks like the new code doesn't handle self correctly. The old code counted from the end, so self was ignored (slightly incorrect. I helped write on the old code, but it's been quite some time). But the new code counts from the start, so it has to account for self.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 18, 2021

Hi @jagerman, could you please take a look? This is a really tiny reproducer. Looks like an edge case that's not handled correctly, maybe something simple like an off-by-one. I'm guessing a minor tweak will fix it, the trick is to know where. I'm hoping that you as the original author may have a clue straightaway.

@rwgk rwgk force-pushed the tensorstore_dim_breakage branch from 456e214 to 32627fc Compare November 19, 2021 15:06
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 19, 2021

I expanded the new test in a trivially systematic way, placing the py::kw_only at different positions:

0: *, int i
1: int, *, int i
2: int, int, *, int i

Then defing (A) a simple function and (B) init.
Calling with i=1 works consistently for (A), fails for (B) 0, but works for (B) 1 & 2.

@jagerman does that give you any clues?

@jagerman
Copy link
Member

I will look into this later today. I think you're right that there's some edge case being triggered here but will likely take some debugging through it to figure it out.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 20, 2021

Issue fixed by @jagerman's PR #3488 :-)

@rwgk rwgk closed this Nov 20, 2021
@rwgk rwgk deleted the tensorstore_dim_breakage branch November 20, 2021 17:31
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.

3 participants