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

Make mcsolve more memory efficient #1669

Merged
merged 2 commits into from
Sep 27, 2021
Merged

Conversation

Ericgig
Copy link
Member

@Ericgig Ericgig commented Sep 21, 2021

Presently mcsolve always save the final state of all trajectories, needed or not.
With this PR, it will only save them when needed.

Related issues or PRs
closes #1667

Changelog
Improve mscolve memory efficiency when states are not needed.

mcsolve would always save the final state of all trajectories,
needed or not, Now only save them when needed.
Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @Ericgig.

Comment on lines -42 to +43
complex[:,::1] states_out
complex[:,::1] ss_out
object states_out
object ss_out
Copy link
Member

Choose a reason for hiding this comment

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

This feels like it might possibly have some knock-on performance issues due to less perfect indexing. I doubt it will, because these aren't inner loop variables, but could we check? I also couldn't immediately see why they needed to be changed at all, but that may just be me missing things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Inside loop I used cdef complex[:,::1] states_out = self.states_out to ensure no impact on performance.

I changed it to keep the numpy array alive. We create it with self.states_out = np.zeros(...) in a function and do np.array(self.states_out) in another and I am not sure it is a safe usage for reference counting. There haven't been any issues, but I preferred to use only one ndarray just in case.

Also np.array(self.states_out) don't work on not initialized memoryview, so we would have to branch the return, but as an object, it is None when not used. (Which I should have mode explicit...)

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that assigning to the memoryview doesn't keep the numpy array alive - I thought the base object got stored within the view, to avoid problems with that (similar to how numpy views keep references to their underlying objects to prevent the memory going stale).

Either way, I'm not actually bothered about it, since as you say, there shouldn't be any performance issues.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 65.88% when pulling af80846 on Ericgig:mcsolve_mem1 into 9b235c4 on qutip:master.

@hodgestar
Copy link
Contributor

@Ericgig Looks good. Should this go into 4.6.3 or only 4.7?

@hodgestar
Copy link
Contributor

@Ericgig Could you add a changelog entry to the PR description before merging?

@Ericgig Ericgig added this to the QuTiP 4.7 milestone Sep 27, 2021
@Ericgig Ericgig merged commit 5a344e3 into qutip:master Sep 27, 2021
@Ericgig Ericgig deleted the mcsolve_mem1 branch October 27, 2021 20:03
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.

mcsolve always store the final state.
5 participants