Skip to content

BUG: Fix default fallback in genfromtxt#16242

Merged
mattip merged 2 commits intonumpy:masterfrom
seberg:genfromtxt-default-regression
May 19, 2020
Merged

BUG: Fix default fallback in genfromtxt#16242
mattip merged 2 commits intonumpy:masterfrom
seberg:genfromtxt-default-regression

Conversation

@seberg
Copy link
Member

@seberg seberg commented May 14, 2020

This affected (for example?) if the dtype=object was used
without a converter, meaning that the default one is used.
And this is currently the last one, which is string_ (and thus
bytes).

Closes gh-16189

@seberg seberg force-pushed the genfromtxt-default-regression branch 2 times, most recently from fba8b63 to b802086 Compare May 14, 2020 22:25
@seberg seberg modified the milestone: 1.19.0 release May 18, 2020
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM - one comment about a comment perhaps falling out-of-sync with the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The preceding comment should be updated as well I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! done.

This affected (for example?) if the `dtype=object` was used
without a converter, meaning that the default one is used.
And this is currently the last one, which is `string_` (and thus
bytes).

Closes numpygh-16189
@seberg seberg force-pushed the genfromtxt-default-regression branch from b802086 to 8bc4701 Compare May 18, 2020 21:44
Comment on lines 511 to +513
(nx.integer, int, -1),
(nx.floating, float, nx.nan),
(nx.complexfloating, complex, nx.nan + 0j),])
(nx.complexfloating, complex, nx.nan + 0j),
Copy link
Member

Choose a reason for hiding this comment

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

In what circumstance are these actually used? What fails if you just remove these three rather than moving two to the bottom.

(out of scope for this PR, but curious

Copy link
Member Author

Choose a reason for hiding this comment

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

They should b be used for other dtypes such as i4 or float16 which are not specifically listed here. I guess the real reason why not to list all of them and rather have these is that it was the smaller change. (there are test failures if you delete this), but only for the integer case.

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@mattip mattip merged commit 249198b into numpy:master May 19, 2020
@mattip
Copy link
Member

mattip commented May 19, 2020

Thanks @seberg

@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Jul 8, 2020
@seberg seberg removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 8, 2020
@seberg seberg deleted the genfromtxt-default-regression branch July 8, 2020 14:49
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.

BUG: genfromtxt produce complex values with dtype object

5 participants