Skip to content

Comments

ENH: fixing generic error messages to be more specific in multiarray/descriptor.c#15484

Merged
mattip merged 24 commits intonumpy:masterfrom
ericmariasis:report_generic
Feb 2, 2020
Merged

ENH: fixing generic error messages to be more specific in multiarray/descriptor.c#15484
mattip merged 24 commits intonumpy:masterfrom
ericmariasis:report_generic

Conversation

@ericmariasis
Copy link
Contributor

@ericmariasis ericmariasis commented Feb 1, 2020 •
edited by eric-wieser
Loading

Closes #15316

@eric-wieser
Copy link
Member

eric-wieser commented Feb 1, 2020

If there are no callers left, can you delete the definition of _report_generic_error?

eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
ericmariasis and others added 5 commits February 1, 2020 07:23
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
0d4b5af 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
46428cc 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
60da9eb 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis
Review actions for issue numpy#15484
5bb389f
@ericmariasis
Merge branch 'report_generic' of https://github.com/ericmariasis/numpy …
ea28bc2 
…into report_generic

* 'report_generic' of https://github.com/ericmariasis/numpy:
  Update numpy/core/src/multiarray/descriptor.c
  Update numpy/core/src/multiarray/descriptor.c
  Update numpy/core/src/multiarray/descriptor.c
@ericmariasis
Copy link
Contributor Author

ericmariasis commented Feb 1, 2020

If there are no callers left, can you delete the definition of _report_generic_error?

I was so close but unfortunately there was one location that I couldn't think of a good replacement for. It was basically if the data type could not be recognized in general.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 1, 2020

If it's one location, you could just inline the function

@ericmariasis
Copy link
Contributor Author

ericmariasis commented Feb 1, 2020

If it's one location, you could just inline the function

Done.

@ericmariasis ericmariasis reopened this Feb 1, 2020
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
ericmariasis and others added 3 commits February 1, 2020 08:55
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
dfb2d34 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
b7e53b3 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
c4163fa 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
ericmariasis and others added 5 commits February 1, 2020 10:03
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
95ace5d 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
662be74 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
e4477c7 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
c4b8e25 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
@ericmariasis
more review actions
d8f865f
eric-wieser
eric-wieser reviewed Feb 1, 2020
View reviewed changes
@ericmariasis @eric-wieser
Update numpy/core/src/multiarray/descriptor.c
9bbe90d 
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
eric-wieser
eric-wieser approved these changes Feb 1, 2020
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good. Probably doesn't need a test, but it would be great if you could post some example error output in a comment here.

@ericmariasis
Copy link
Contributor Author

ericmariasis commented Feb 1, 2020

Closes #15316

Example error output:

dtype2 = np.dtype([2,3,4])
Traceback (most recent call last):
File "", line 1, in
TypeError: data type not understood

dtype3 = np.dtype((float,))
Traceback (most recent call last):
File "", line 1, in
TypeError: data type not understood

-- The above is now updated to give better details of the error.

seberg
seberg reviewed Feb 1, 2020
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

@ericmariasis can you paste the error that happens with this patch applied? Thought ticks around '%R' may make sense. I am sure you can always massage the text, but its a nice improvement.

Some of the lines are a bit long, we often use:

PyErr_Format(PyExc_TypeError,
        "8 spaces indentation before the message", )

style. It does not have to be that style, below 80 characters would be nice though (at least not much over).

ericmariasis and others added 6 commits February 1, 2020 13:16
@ericmariasis @seberg
Update numpy/core/src/multiarray/descriptor.c
991a787 
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
@ericmariasis @seberg
Update numpy/core/src/multiarray/descriptor.c
54efe37 
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
@ericmariasis @seberg
Update numpy/core/src/multiarray/descriptor.c
303ce30 
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
@ericmariasis
seberg review actions
a0336e2
@ericmariasis
review actions
1542f59
@ericmariasis
resolved merge conflict
812920c
seberg
seberg reviewed Feb 1, 2020
View reviewed changes
seberg
seberg approved these changes Feb 2, 2020
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, I am good with putting it in now. If someone wants to massage the error messages also great, but most of them are rare, and they read fine to me.
Two more style nitpicks if you are still motivated.

@ericmariasis
Copy link
Contributor Author

ericmariasis commented Feb 2, 2020

Now that all reviewers have approved, is there anything on my part I can do to help this get merged into master?

@mattip mattip merged commit cd82df3 into numpy:master Feb 2, 2020
@mattip
Copy link
Member

mattip commented Feb 2, 2020

Thanks @ericmariasis

@ericmariasis ericmariasis deleted the report_generic branch February 2, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg approved these changes

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

Replace calls to _report_generic_error with a more useful error message.

4 participants

@ericmariasis @eric-wieser @mattip @seberg