Repository navigation
Change BaseFrame.__setattr__ to be fast for private attributes. - #6940
Conversation
|
Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
| for representation_attr in self.representation_info.values(): | ||
| repr_attr_names.update(representation_attr['names']) | ||
|
|
||
| if attr in repr_attr_names: |
There was a problem hiding this comment.
The logic has changed in that this block is now inside the above if hasattr(..) block. Are you sure this is correct?
There was a problem hiding this comment.
Yes, since we know that without that block, repr_attr_names is an empty set.
|
Looks great, but you still need another factor of 6 to get to the speed of the template idea (and I would need to re-profile that with this fix in place). 😄 |
|
Test failure is unrelated (open file of some web thingy). |
|
@taldcroft - see #6941 for a factor 50(!). p.s. Just in case: unlike for #6941, for this PR I have no qualms about implementation at all; it should just go in, as it speeds up without any downsides. |
|
@adrn - would you be able to do a quick review? This small change is not controversial, and while it is about performance, there seems to be no reason not to have it in 3.0. |
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
Agree this is not controversial, so will merge now.
|
Can we also backport it to LTS? |
|
@bsipocz - I guess no real reason not to. In a way it was a bug. |
Change BaseFrame.__setattr__ to be fast for private attributes.
4 participants
Add one line and get a speed-up of more than a factor 3 in slicing; not bad!
But is also shows the risk inherent in overriding
__getattr__and__setattr__. Here, just to protect the user from accidentally writing an attribute with the same name as that of a representation coordinate, we slowed down things considerably.And of course this is partially why I'm hesitant to try to paper over simple mistakes such as this by caching yet more stuff (though caching the representation names set seems an obvious next fix).