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

Fix high-level ak.Array.__dir__ to include methods and properties of overridden classes. #993

Merged
merged 2 commits into from Jul 14, 2021

Conversation

jpivarski
Copy link
Member

@nsmith- This might have been preventing Jupyter from displaying custom methods and properties in tab-completion.

Let me know if you think this implementation is right. (I'm not including the underscored attributes of type(self) because this __dir__ should be showing an object's attributes; it should not include class attributes like __mro__.)

Copy link
Contributor

@nsmith- nsmith- left a comment

Choose a reason for hiding this comment

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

This adds all methods and properties found in type(self) that don't start with underscore. Jupyter/ipython tab-completion filters underscore methods anyway, so strictly speaking that is optional. (actually if you press tab after adding an underscore the attributes re-appear) Normally __dir__ does include such items.
Additional ipython tab-completion is available for __getitem__ if _ipython_key_completions_ is declared, according to https://ipython.readthedocs.io/en/stable/config/integrating.html#tab-completion

@jpivarski
Copy link
Member Author

Filtering out the methods that start with underscore is optional for Jupyter/IPython tab completion, but declaring in __dir__ that an object has, for example, a __mro__ attribute would be wrong. In general, type(self) can have different attributes than self, so advertising type(self)'s attributes in self's __dir__ is technically wrong. However, all non-underscored attributes of a class are considered bound to the object as well (e.g. all of the properties, methods, classmethods, and staticmethods), so I don't anything wrong could be claimed if we limit it to the non-underscored ones.

Why am I mixing in dir(type(self)) at all? You make subclasses of ak.Array and ak.Record with special attributes, but as subclasses, __dir__ is already overloaded to not see those attributes. The standard __dir__ is called on ak.Array/ak.Record, but this is already too far up the inheritance tree to see the special attributes you've made. That's why we were losing them before.

Alternatively, I suppose I could walk up the MRO asking for the __dict__ at each level, but that's just the sort of thing that feels fragile in Python because there are all sorts of different ways for a Python object to get "effective" attributes. __slots__, for instance.

This method will not include ad-hoc attributes added to a particular array/record instance, but that's not a good pattern anyway, considering how easily instances are destroyed and created. As defined, __dir__ describes attributes relevant for an array's type.

Thanks for the pointer to _ipython_key_completions_. According to the documentation, it defers to __dir__ if there is no _ipython_key_completions_, and that's how I would want to define this, anyway. The need to expand __dir__ came up for a non-Jupyter reason (identifying that an array of vectors has "px", "py", "pz", "E" without actually computing these properties in scikit-hep/fastjet). I was asking you because I thought this might impact NanoEvents.

But if nothing looks dangerous for you, give me a thumbs-up and I'll merge it. Thanks!

@nsmith-
Copy link
Contributor

nsmith- commented Jul 13, 2021

According to the documentation, it defers to __dir__ if there is no _ipython_key_completions_

Er, one is for completing getattr, and the other for getitem I thought?
Otherwise things look ok, I missed the point about dir(type(self)) returning some inappropriate attributes.

@jpivarski
Copy link
Member Author

Er, one is for completing getattr, and the other for getitem I thought?

Oh, I missed that (two separate paragraphs in the IPython docs). Yes, adding an explicit _ipython_key_completions_ to return fields would make it more useful. I'll add that to this PR and then enable auto-merge.

Thanks for checking!

@jpivarski jpivarski merged commit b631667 into main Jul 14, 2021
@jpivarski jpivarski deleted the jpivarski/fix-highlevel-Array-dir branch July 14, 2021 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants