Skip to content

MAINT: Refactor dtype conversion functions to be more similar - #15300

Merged
seberg merged 4 commits into
numpy:masterfrom
eric-wieser:tidy-dtype-ctors-try_convert
Jan 10, 2020
Merged

seberg merged 4 commits into
numpy:masterfrom
eric-wieser:tidy-dtype-ctors-try_convert

Conversation

@eric-wieser

Copy link
Copy Markdown
Member

Before there were three signatures:

  • PyArray_Descr*(...)
  • PyArray_Descr*(..., int* err_flag)
  • int(..., PyArray_Descr** ret)

This unifies them all into PyArray_Descr*(...), and introduces a special singeton to distinguish "error" from "gave up" cases.

Also changes function names for consistency.


The goal here is to make it very easy to spot returns that forgot to set an error.

@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-try_convert branch 3 times, most recently from d11910b to 4ae10a4 Compare January 9, 2020 14:13
Comment thread numpy/core/src/multiarray/descriptor.c Outdated
@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-try_convert branch from 4ae10a4 to eb84a9b Compare January 9, 2020 14:26
Before there were three signatures:

* `PyArray_Descr*(...)`
* `PyArray_Descr*(..., int* err_flag)`
* `int(..., PyArray_Descr** ret)`

This unifies them all into `PyArray_Descr*(...)`, and introduces a special singeton to distinguish "error" from "gave up" cases.

Also changes function names for consistency.
@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-try_convert branch from eb84a9b to 2877369 Compare January 9, 2020 14:37
@eric-wieser
eric-wieser requested a review from seberg January 9, 2020 15:30
@seberg

seberg commented Jan 9, 2020

Copy link
Copy Markdown
Member

Is this an intermediate step or do you expect that this "address" will stick around? Mainly curious, in other places I think we use NotImplemented for similar purposes, although a static address may have its advantages.

@eric-wieser

Copy link
Copy Markdown
Member Author

do you expect that this "address" will stick around?

Yes

think we use NotImplemented for similar purposes

I thought about this, but it would require some annoying (PyArray_Descr*) casts, and I'd feel obliged to INCREF/DECREF it.

I can update this patch to use that solution if you'd prefer.

@seberg

seberg commented Jan 9, 2020

Copy link
Copy Markdown
Member

Yeah, I realize that is not ideal either. So dunno, I was mainly toying with the thought of making it a small function similar to error_occured.
Maybe if (_is_result_or_error(ret)) {return res} but right now I doubt its better. attempt_failed read also a bit like an error; maybe we will have a nice idea in a bit...

@eric-wieser

Copy link
Copy Markdown
Member Author

Updated with a fixup commit to use NotImplemented - it's not that bad, and the semantics are definitely sensible.

@seberg

seberg commented Jan 9, 2020

Copy link
Copy Markdown
Member

Brief glance... Yeah looks reasonable, and probably a more expected pattern at least. I will look over it (and I assume merge it) tonight, probably.

@eric-wieser

Copy link
Copy Markdown
Member Author

And one more fixup to make the _array_descr-prefixed one have the same API.

Comment on lines -489 to +487
Py_DECREF(conv);
}
Py_DECREF(conv);

@eric-wieser eric-wieser Jan 9, 2020 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is probably quite a good argument for why I'm INCREFing Py_NotImplemented before returning it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Not incrementing is just too smart anyway...

@eric-wieser

Copy link
Copy Markdown
Member Author

CI now passing, silly typo.

If this looks ready, probably best to squash-merge

@seberg

seberg commented Jan 10, 2020

Copy link
Copy Markdown
Member

Thanks, I let me put this in. Definitely an improvement. I like the try_ naming convention.

Not that I expect it, but when you are done with this, I will try to remember running a ref-count checker over the tests again. Better than being too paranoid about too.

@seberg
seberg merged commit c4d5e12 into numpy:master Jan 10, 2020
@seberg

seberg commented Jan 10, 2020

Copy link
Copy Markdown
Member

oooops, I forgot to squash merge, damn :(. Oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants