-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add solution downsampling feature #537
base: master
Are you sure you want to change the base?
Conversation
2212ffe
to
00ae4a3
Compare
I have looked over this with @aymkhalil and it seems like a good first step. In terms of design, a downside is that it requires creating new Solution, State, and DA objects. But this leads to the advantage of not making any modification to the read and write functions themselves. One important question is whether to keep the scikit-image optional dependency or replace it with some complicated numpy code (which seems like it would necessarily introduce a copy, at least in 3D). An obvious next step after this is to allow different types of output (e.g. "checkpoints" and smaller ones) with different frequencies. @mandli, any comments? |
Reviewing, I will try to send some feedback by tonight. One thought though after reading through the PR once was that there are a couple of spots where there are deviations from Python coding styles which could be corrected easily. I will try to point these out with some code comments. |
- ds_domain_ranges: global ranges of the downsampled solution | ||
""" | ||
|
||
from skimage.transform import downscale_local_mean |
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 we add some specifics as to what downscale_local_mean
is doing? For instance, what the stencil size is? At the very least put in the doc-string that the function is from sci-kit image.
I think this generally looks good. I would suggest that tests be added since this is a fairly complex procedure, especially in parallel. I also wonder if this functionality should be implemented as a separate function that takes in a solution and then returns the specified downsampled solution. I only say this as most of the functionality ends up passing through |
@mandli I'm working on the test cases and your comments and will push the new modifications soon. Regarding your last comment about implementing the functionality in a separate module, I agree with you, however, I just wanted the downsampled version of the solution to stick around in memory as long as the original solution object lives to save the overhead of initializing the new DA objects should the write function be called several times during the simulation. Now implementing a different technique for downsampling will require some refactoring, I agree, but at least the average method (which is performed locally only) is already separate in the Now this leaves us with Anyway, let's have another round of comments after I push my changes. |
@aymkhalil You make some good points and the core routine, There is some concern I have regarding memory pressure but as that is usually not an issue for us I am not too worried about it. |
3aba659
to
d451c50
Compare
d451c50
to
4fa125f
Compare
If the Then we could do the following:
However, other methods in
Some methods may require non-overlapping blocks, overlapping windows, padding, etc. and this needs more discussion as to what methods are expected and what techniques they have in common to take this into account as @mandli has pointed. @ketch do you want this as part of this PR? |
Thanks for putting in the tests, just a few more comments:
|
This PR enables downsampling a solution to save on write time & disk space should the solution be written out to files. It uses local averaging over user defined averaging factors along each axis: