Skip to content
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.

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

DEP: deprecate undocumented, unused dtype type dicts #11382

Merged
merged 1 commit into from
Jul 14, 2018

Conversation

mattip
Copy link
Member

@mattip mattip commented Jun 19, 2018

Fixes #11341 by removing sctypeNA and typeNA, left over from long ago.
Will cause a merge conflict with PR #11340

Edit:: strike out merge conflict, that PR was merged already

@eric-wieser
Copy link
Member

Think API: might be more appropriate - this isn't just a deprecation

@mattip mattip changed the title DEP: remove undocumented, unused dtype type dicts API: remove undocumented, unused dtype type dicts Jun 19, 2018
@mattip
Copy link
Member Author

mattip commented Jun 19, 2018

It should get a release note

@eric-wieser
Copy link
Member

This probably needs to hit the mailing list.

@mattip
Copy link
Member Author

mattip commented Jun 24, 2018

The only response on the mailing list was from @seberg asking if it should go through a deprecation cycle. How would we do that? If we want to deprecate accessing the dictionaries, we would have to somehow wrap the __getattr__ of ... ? They live in numpy.core.numerictypes, are exported to numpy.core in its __init__.py via __all__ += numeric.__all__. Or we could wrap the getitem in the dictionary itself to emit a warning on access.

@eric-wieser
Copy link
Member

Python 3.7 adds module.__getattr__, so we do have a path to eventual deprecation.

The easy solution here seems to be just to deprecate item access.

@mattip mattip changed the title API: remove undocumented, unused dtype type dicts DEP: deprecate undocumented, unused dtype type dicts Jun 25, 2018
@mattip
Copy link
Member Author

mattip commented Jun 25, 2018

I added deprecation warnings to the __getitem__ and get methods, which should cover 90% of the use cases, I guess we could do __missing__, pop, and a few others as well but it seems like overkill.

'of numpy', DeprecationWarning, stacklevel=2)
return dict.__getitem__(self, key)
def get(self, key, default=None):
# 2018-June-24, 1.16
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere we use 2018-06-24, which is ISO-8601

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

def test_typeNA(self):
# Ticket #31
assert_equal(np.typeNA[np.int64], 'Int64')
assert_equal(np.typeNA[np.uint64], 'UInt64')
Copy link
Member

Choose a reason for hiding this comment

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

Better to mask the deprecations and keep the test here.

Copy link
Member Author

Choose a reason for hiding this comment

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

adopted

@mattip
Copy link
Member Author

mattip commented Jul 3, 2018

ping

class TypeNADict(dict):
def __getitem__(self, key):
# 2018-06-24, 1.16
warnings.warn('sctypeNA and typeNA will be removed in v1.17 '
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to commit to this fast a deprecation cycle? If so, this needs a VisibleDeprecationWarning probably. Might be worth getting more feedback on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

will add the visible warning and delay to 1.18

@@ -504,3 +504,12 @@ class TestGeneratorSum(_DeprecationTestCase):
# 2018-02-25, 1.15.0
def test_generator_sum(self):
self.assert_deprecated(np.sum, args=((i for i in range(5)),))

class TestSctypeNA(_DeprecationTestCase):
# 2018-June-24, 1.16
Copy link
Member

Choose a reason for hiding this comment

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

nit: iso 8601 again

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@charris
Copy link
Member

charris commented Jul 3, 2018

A deprecation is the way to go. I think the NA stands for numarray and it is just possible that some folks, I'm thinking STScI, could possibly have missed some things when updating before we removed the numarray compatibility module.

@mattip
Copy link
Member Author

mattip commented Jul 4, 2018

@charris

A deprecation is the way to go

Without any DeprecationWarning, just remove it?

@charris
Copy link
Member

charris commented Jul 4, 2018

A deprecation is a DeprecationWarning. Removal is removal :)

@eric-wieser
Copy link
Member

@mattip: I don't think @charris was asking you to change from VisibleDeprecationWArning to DeprecationWarning there

@charris
Copy link
Member

charris commented Jul 5, 2018

I'm good with either, I just wanted to add my vote for some sort of deprecation.

@mattip mattip force-pushed the remove-typeNA branch 2 times, most recently from 0a20709 to 5f766a1 Compare July 6, 2018 05:19
@mattip
Copy link
Member Author

mattip commented Jul 6, 2018

leave as VisualDeprecationWarning, rebased off master, and squashed

@@ -14,6 +14,12 @@ New functions
Deprecations
============

`typeNA` and `stypeNA` have been deprecated
Copy link
Member

Choose a reason for hiding this comment

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

typo: sctypeNA

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

def show(_dict, key):
return _dict[key]
self.assert_deprecated(show, args=(np.core.sctypeNA, '?',))
self.assert_deprecated(show, args=(np.core.typeNA, '?',))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is better than just self.assert_deprecated(lambda: np.sctypeNA['?'])

Copy link
Member Author

Choose a reason for hiding this comment

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

adopted

@@ -47,8 +47,10 @@ def test_pickle_transposed(self):

def test_typeNA(self):
# Ticket #31
Copy link
Member

Choose a reason for hiding this comment

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

Change this to gh-515 while you're here, maybe

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@mattip
Copy link
Member Author

mattip commented Jul 14, 2018

ping. This seems non-cotroversial.

-------------------------------------------

The type dictionaries `numpy.core.typeNA` and `numpy.core.sctypeNA` were buggy
and not documented. They will be removed in the next release. Use
Copy link
Member

Choose a reason for hiding this comment

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

2 releases, not the next release

Copy link
Member

Choose a reason for hiding this comment

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

the actual deprecation message is correct (1.18) so just needs correcting here

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@mattip
Copy link
Member Author

mattip commented Jul 14, 2018

squashed to a single commit

@rgommers rgommers merged commit c8b246c into numpy:master Jul 14, 2018
@rgommers
Copy link
Member

LGTM now and CI still happy (except Appveyor didn't start yet). Merged. Thanks @mattip

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.

None yet

4 participants