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

Hint for efficiency #1300

Open
thesamovar opened this issue May 10, 2021 · 2 comments
Open

Hint for efficiency #1300

thesamovar opened this issue May 10, 2021 · 2 comments

Comments

@thesamovar
Copy link
Member

I had a student today who had some code running very slowly. It turned out he was accessing M.v[i, t] for i and t in a loop, and M a StateMonitor. Turning this into a single v=M.v and then v[i,t] made it run almost instantly. I'm wondering if we can catch this sort of thing and warn about it because it might be tripping up a few people. Perhaps something like if you access M.v more than 100 times as often as you run M it gives you the warning? Not sure of the best way here, or if there's some general way to catch these sorts of issues or not.

@thesamovar
Copy link
Member Author

Maybe it would just be better to cache that getattr so that if nothing has changed it doesn't need to do any work?

@mstimberg
Copy link
Member

Hmm, not sure about a warning, I'd prefer if it would be fast in the first place :) Of course there's almost never a reason to loop over a 2D array instead of treating it like a matrix, but many users will still do it, and there is no reason to make this slow if we don't have to.
I just had a quick look into StateMonitor.__getattr__ and there's a weird thing: if you access M.v (i.e. with units), it will make a copy of the data, whereas it gives you a direct reference without a copy if you access M.v_ (i.e. without units). So the latter should already be reasonably fast even in looped access.

I could come up with reasons for a copy – e.g. related to problems when the underlying data gets resized in a second run – but I don't quite see why it should only apply to values with units... I'll try to dig into history at some point to see whether this was intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants