-
Notifications
You must be signed in to change notification settings - Fork 126
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
Reducing memory errors in dict_to_netcdf #1095
Conversation
… files Previously, an array was created with the total number of unique timesteps, which could become very large when lots of particles were deleted at different times. Access rows and columns were later deleted. This has now been changed so that the initial data array is much smaller and less likely to get out of Memory errors
As not needed anymore in new `ParticleFile.read_from_npy()` implementation
As it's needed for NoteList ParticleSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes will create some headache for other running PRs, especially the ones concerning non-indexable particle sets.
That said: it does seem to work as you intend it to work, and I understand the motivation behind the change. So, yeah: fine with this implementation. Can be merged.
@@ -32,7 +32,9 @@ def _to_write_particles(pd, time): | |||
"""We don't want to write a particle that is not started yet. | |||
Particle will be written if particle.time is between time-dt/2 and time+dt (/2) | |||
""" | |||
return [i for i, p in enumerate(pd) if time - np.abs(p.dt/2) <= p.time < time + np.abs(p.dt) and np.isfinite(p.id)] | |||
return [i for i, p in enumerate(pd) if (((time - np.abs(p.dt/2) <= p.time < time + np.abs(p.dt)) | |||
or (np.isnan(p.dt) and np.equal(time, p.time))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this part ? Where does that come from ? How could that condition occur ?
& np.greater_equal(time + np.abs(pd['dt'] / 2), pd['time'], where=np.isfinite(pd['time'])) | ||
return ((np.less_equal(time - np.abs(pd['dt']/2), pd['time'], where=np.isfinite(pd['time'])) | ||
& np.greater_equal(time + np.abs(pd['dt'] / 2), pd['time'], where=np.isfinite(pd['time'])) | ||
| ((np.isnan(pd['dt'])) & np.equal(time, pd['time'], where=np.isfinite(pd['time'])))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this part ? Where does that come from ? How could that condition occur ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because particle.dt
is not set yet before a call to pset.execute; so previously we couldn't write particles before execution. Because of a bug in the unit tests (now fixed), this wasn't picked up
data = np.nan * np.zeros((self.maxid_written+1, time_steps)) | ||
time_index = np.zeros(self.maxid_written+1, dtype=np.int64) | ||
t_ind_used = np.zeros(time_steps, dtype=np.int64) | ||
maxtime_steps = max(time_steps.values()) if time_steps.keys() else 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here the variable name is misleading - is it really the max. 'timestep' or rather the max. 'time' itself ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've now renamed to n_timesteps
and max_timesteps
time_index = np.zeros(len(time_steps)) | ||
id_index = {} | ||
count = 0 | ||
for i in sorted(time_steps.keys()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorting the field here can potentially be quite time-consuming ... especially with millions of elements. But if your approach requires it: ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we only do this ParticleFile.export()
once. If we want reproducibility of the output file (i.e. the particles IDs increase with row number), then it's important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k, I see that. Obviously, for ordered-collection psets, I would not do that sorting (as the collection is pre-sorted). That said, for AoS and SoA, one may need to do that trick indeed.
This PR is a rewrite of
read_from_npy
so that it has a smaller memory footprint.Previously, an array was created with the total number of unique timesteps, which could become very large when lots of particles were deleted at different times. Excess rows and columns were later deleted. This has now been changed so that the initial data array is much smaller and less likely to get out of Memory errors
This PR is in parallel to #1092, which would still be needed on very large files