Skip to content

ENH: Ensure dispatcher TypeErrors report original name #21731

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2022
Merged

ENH: Ensure dispatcher TypeErrors report original name #21731

merged 1 commit into from
Jun 13, 2022

Conversation

seberg
Copy link
Member

@seberg seberg commented Jun 11, 2022

This replaces the name in the TypeError with the actually raised
name. In principle we could add one more check, because a
signature related TypeError must have a traceback with exactly
one entry (so sys.exc_info()[2].tb_next is None).
In practice this seems unnecessary though.

This ensures the following message:

>>> np.histogram(asdf=3)
TypeError: histogram() got an unexpected keyword argument 'asdf'

Closes gh-21647

@seberg seberg mentioned this pull request Jun 11, 2022
Closed
@shoyer
Copy link
Member

shoyer commented Jun 11, 2022

Does this add any measureable slowdown in the case where there is no error?

@seberg
ENH: Ensure dispatcher TypeErrors report original name
8fabdec 
This replaces the name in the TypeError with the actually raised
name.  In principle we could add one more check, because a
signature related TypeError must have a traceback with exactly
one entry (so `sys.exc_info()[2].tb_next is None`).
In practice this seems unnecessary though.

This ensures the following message:

    >>> np.histogram(asdf=3)
    TypeError: histogram() got an unexpected keyword argument 'asdf'

Closes numpygh-21647
@seberg seberg force-pushed the array-function-fixup-exc branch from 7079865 to 8fabdec Compare June 11, 2022 02:41
@seberg
Copy link
Member Author

seberg commented Jun 11, 2022

Hmmm, the benchmarks give something like:

       before           after         ratio
     [4cb68893]       [8fabdecb]
     <main>           <array-function-fixup-exc>
+         292±1ns          307±2ns     1.05  bench_overrides.ArrayFunction.time_mock_broadcast_to_numpy
          331±6ns          346±5ns     1.05  bench_overrides.ArrayFunction.time_mock_broadcast_to_duck
          487±9ns         498±10ns     1.02  bench_overrides.ArrayFunction.time_mock_concatenate_mixed
         401±10ns          410±3ns     1.02  bench_overrides.ArrayFunction.time_mock_concatenate_numpy
      1.44±0.01μs      1.45±0.01μs     1.01  bench_overrides.ArrayFunction.time_mock_concatenate_many
          437±7ns         441±10ns     1.01  bench_overrides.ArrayFunction.time_mock_concatenate_duck

(not super reliable on these time scalas, but that is the rough idea I guess)

So it is measurable on the cheapest functions (those functions are as cheap as it gets).
In Python 3.10 it seems less but probably because Python 3.10 seems slower (I will guess **kwarg handling got worse because its a niche feature in a sense).

@seberg
Copy link
Member Author

seberg commented Jun 11, 2022

Well, not sure if its worth it, I don't like the weird name, of course completely incorrect signatures are maybe not super common. The question is really whether this confuses new users or not (and I have no idea).
I don't hate the dispatcher_name == function_name semi-fix to be honest, and it would align at least most of them.

mhvk
mhvk approved these changes Jun 11, 2022
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Looks very good to me! Happy that it was not too much work.

@shoyer
Copy link
Member

shoyer commented Jun 11, 2022

Personally I would lean against this solution -- I'm not sure the slight overhead in every NumPy function call is worth it.

@mhvk
Copy link
Contributor

mhvk commented Jun 11, 2022 •
edited
Loading

Note that in newest python (3.11) there is no overhead whatsoever for a try/except.

@mattip mattip added the triage review Issue/PR to be discussed at the next triage meeting label Jun 13, 2022
@charris
Copy link
Member

charris commented Jun 13, 2022

I'm OK with this, especially if the try..except is as fast as claimed in Python 3.11

“Zero-cost” exceptions are implemented. The cost of try statements is almost eliminated when no exception is raised. (Contributed by Mark Shannon in bpo-40222.)

@shoyer
Copy link
Member

shoyer commented Jun 13, 2022

I'm OK with this, especially if the try..except is as fast as claimed in Python 3.11

“Zero-cost” exceptions are implemented. The cost of try statements is almost eliminated when no exception is raised. (Contributed by Mark Shannon in bpo-40222.)

agreed!

@charris
Copy link
Member

charris commented Jun 13, 2022

agreed!

Let's put this in, then. Thanks Sebastian.

@charris charris merged commit 80e55b2 into numpy:main Jun 13, 2022
@charris charris removed the triage review Issue/PR to be discussed at the next triage meeting label Jun 13, 2022
@seberg seberg deleted the array-function-fixup-exc branch June 13, 2022 17:08
This was referenced Jan 17, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
01 - Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENH: Report the correct function name for incorrect arguments
5 participants
@seberg @shoyer @mhvk @charris @mattip