-
Notifications
You must be signed in to change notification settings - Fork 280
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
Fast seek through file #4736
Fast seek through file #4736
Conversation
a50d5dd
to
0bbac11
Compare
8a4212b
to
921d84f
Compare
@yt-fido test this please |
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.
Just great work! Thanks for such a detailed PR description -- and great changes.
@yt-fido test this please |
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 can't say in all honesty that I understand all of it, but I also cannot say that about the code this is replacing. I'm alright with this going in as long as tests pass, given how clear the gain is.
cpu_list = [self.domain_id - 1] | ||
fill_hydro( | ||
fd, | ||
file_handler.offset, | ||
file_handler.level_count, | ||
cpu_list, | ||
levels, | ||
level_inds, |
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.
can you explain why you renamed this variable (as well as domain_inds
) ?
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.
If you read the line just below, you'll see cell_inds
and file_inds
. Before, I got confused by the different naming conventions and I had to read the code to actually understand all those arrays have the same shape. To make it more consistent, I renamed all the arrays to be whatever_inds
.
This provides additional speedups on top of #4720. We should probably wait for the former to be merged before reviewing this one.
The two improvements are:
Timings
Using the same script as in #4720
main
(s)Using the same script as in #4720, but centring on the 10 kpc around the largest overdensity (located at
[0.50303078, 0.49916649, 0.49960136]
). This still includes ~800,000 cells.In this region, most of the cells are at the highest resolution, which means the low-resolution regions can be skipped when reading the hydro files.
main
(s)If we sum the AMR + hydro, we get the following:
Note that, when using #4734, we only get a 2s penalty for dropping the
bbox
argument fromyt.load
(which, in our case, reduces the number of domains to check intersection with from 4096 to 1079).On the cluster I've tested, the throughput is roughly 5µs/cell (200,000 cells/s) which is on the order of 1.5Mib/s. Note that this takes into account reading (a subset of) the AMR files, seeking through the hydro files and concat the resulting arrays with no a priori knowledge of the dataset (i.e. all indexing is coming from reading the files themselves).