-
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
ENH: allow cell_widths and nproc > 1 for load_uniform_grid #4732
ENH: allow cell_widths and nproc > 1 for load_uniform_grid #4732
Conversation
needs tests! but seems to work? |
also seems to have broken many unexpected things :) definitely WIP |
5bd5583
to
554fc83
Compare
554fc83
to
df8efbf
Compare
@yt-fido test this please |
@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.
As long as the test pass I'm okay with this, but I believe that the signature changes in what looks like public API functions should be discussed, or changed for backward compatibility.
left_edges.append(lle) | ||
right_edges.append(lre) | ||
shapes.append(rei - lei) | ||
slices.append(np.s_[lei[0] : rei[0], lei[1] : rei[1], lei[2] : rei[2]]) | ||
|
||
return left_edges, right_edges, shapes, slices | ||
return left_edges, right_edges, shapes, slices, cell_widths_by_grid |
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 looks like a backward incompatible change. Returning cell_widths_by_grid
should be optional, and the default behaviour should remain stable, unless there's a consensus that yt.utilities.decompose
isn't considered public API ?
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.
It's not public API, so this should be ok by me.
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.
oh! ya, I had assumed without too much thought that this was not public API. I think there is a small chance that there's an external frontend out there that utilizes this function. Happy to change this up to cover that scenario: the simplest change might be to have the return from this function vary and include a deprecation, e.g.,
if cell_widths is not None:
return left_edges, right_edges, shapes, slices, cell_widths_by_grid
else:
deprecation warning ("this function will have an additional return value in the future...")
return left_edges, right_edges, shapes, slices
Could leave out the deprecation but that might make future typing efforts annoying.
Or I could add a split_array_with_cell_widths
function and just call that directly from the load_uniform_grid
when needed. That'd probably result in a bit of code duplication, but it might be clearer to have a separate function.
Also happy to leave this change as-is.
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.
Following John's remark I think the best course of action is to keep it simple and treat it as private. We can always inject additional backward-compatibility fixes later if anyone asks for it.
PR Summary
Working on getting
load_uniform_grid
to work with nproc > 1 for stretched grids (whencell_widths
is allowed). Most the changes here are to the grid decomposition.Closes #4330
PR Checklist
Sample from the version just pushed: