DEP: records: Deprecate treating shape=0 as shape=None

[eric-wieser]

shape=n is a shorthand for shape=(n,) except in the case when n==0, when it is a shorthand for shape=None.

This special case is dangerous, as it makes fromrecords(..., shape=len(a)) behave surprisingly when a is an empty sequence.

Users impacted by this warning either:

• Have a bug in their code, and will need to either:
  • wait for the deprecation to expire
  • change their code to use (len(a),) instead of len(a)
• Are using the non-preferred spellling, and will need to replace a literal 0 or False with None

[mattip]

Should hit the mailing list

[mattip]

No tests?

[eric-wieser]

Will add some tests at some point, and hit the mailing list. I doubt we'll get any resistance for deprecating an undocumented, untested, and hard-to-guess API when None is part of the signature and already does the same thing.

[mattip]

If it is not documented and hard to hit, it would be tempting to just change the behaviour without a DeprecationWarning.

[seberg]

I do not care about hitting the mailing list. It is just nonsense, no? Release notes would be good though. We could skip the deprecation by considering it a clear bug. I am not opposed to that, but since the work is already here for the deprecation, might as well go through it maybe.

[WarrenWeckesser]

We could skip the deprecation by considering it a clear bug.

+1

Even though the old code explicitly checks for shape == 0 and handles it the same as shape == None, it smells like a bug. It looks like @mattip and @seberg are in agreement that it is not correct behavior. For example, the third result here should be an array with length 0, not length 3:

In [68]: np.core.records.fromstring(b'ABCDEF', dtype='i1,i1', shape=2)
Out[68]: 
rec.array([(65, 66), (67, 68)],
          dtype=[('f0', 'i1'), ('f1', 'i1')])

In [69]: np.core.records.fromstring(b'ABCDEF', dtype='i1,i1', shape=1)
Out[69]: 
rec.array([(65, 66)],
          dtype=[('f0', 'i1'), ('f1', 'i1')])

In [70]: np.core.records.fromstring(b'ABCDEF', dtype='i1,i1', shape=0)
Out[70]: 
rec.array([(65, 66), (67, 68), (69, 70)],
          dtype=[('f0', 'i1'), ('f1', 'i1')])

The behavior is not documented; indeed, the docstrings for these from... functions (and for the function numpy.rec.array that calls them) are incomplete and say nothing about the parameters.

[eric-wieser]

@seberg, @WarrenWeckesser: can I get you two to agree on whether this should be merged as is, or modified such that the deprecation is skipped?

[WarrenWeckesser]

If there is consensus that this is a bug fix that does not require a deprecation cycle, then removing the deprecation from the PR would be preferable, to avoid future work. It would also simplify how the unit tests will be implemented in the PR. @seberg, what do you think?

[seberg]

I will just merge it as is. Can be followed up with an actual change by the next release IMO. Doing the deprecation (especially in cases that are easy to get rid of it), cannot really hurt.

[mattip]

I think this broke towncrier generation of the release notes since the template does not have a blank line between the end of this bullet list and the link to the PR, it is rendered as

as an array length like any other integer is. The affected functions are:

* `numpy.core.records.fromarrays`
* `numpy.core.records.fromrecords`
* `numpy.core.records.fromstring`
* `numpy.core.records.fromfile`
(`gh-15217 <https://github.com/numpy/numpy/pull/15217>`__)

Not sure how this passed CI ...

[seberg]

Possible that the tests ran when the extra blank line was around (i.e. we merge the ragged-array PR again)