-
Notifications
You must be signed in to change notification settings - Fork 4
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
adds lprec memory estimation and a test #348
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
) | ||
from httomolibgpu.prep.stripe import remove_stripe_based_sorting, remove_stripe_ti | ||
from httomolibgpu.misc.corr import remove_outlier | ||
from httomolibgpu.recon.algorithm import FBP, SIRT, CGLS | ||
from httomolibgpu.recon.algorithm import FBP, LPRec, SIRT, CGLS | ||
from httomolibgpu.misc.rescale import rescale_to_int | ||
|
||
from httomo.methods_database.packages.external.httomolibgpu.supporting_funcs.misc.morph import * | ||
|
@@ -447,7 +447,45 @@ def test_recon_FBP_memoryhook(slices, recon_size_it, ensure_clean_memory): | |
# the estimated_memory_mb should be LARGER or EQUAL to max_mem_mb | ||
# the resulting percent value should not deviate from max_mem on more than 20% | ||
assert estimated_memory_mb >= max_mem_mb | ||
assert percents_relative_maxmem <= 35 | ||
assert percents_relative_maxmem <= 20 | ||
|
||
|
||
@pytest.mark.cupy | ||
@pytest.mark.parametrize("slices", [4, 7]) | ||
def test_recon_LPRec_memoryhook(slices, ensure_clean_memory): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. overall, I think the memory estimator is far from ideal so we can delay with that PR for now. I just wanted to show the potential during the benchmarks. Will come back to it once again after dealing with FBP. thx |
||
data = cp.random.random_sample((1801, slices, 2560), dtype=np.float32) | ||
kwargs = {} | ||
kwargs["angles"] = np.linspace( | ||
0.0 * np.pi / 180.0, 180.0 * np.pi / 180.0, data.shape[0] | ||
) | ||
kwargs["center"] = 500 | ||
kwargs["recon_size"] = 2560 | ||
kwargs["recon_mask_radius"] = 0.8 | ||
|
||
hook = MaxMemoryHook() | ||
with hook: | ||
recon_data = LPRec(cp.copy(data), **kwargs) | ||
|
||
# make sure estimator function is within range (80% min, 100% max) | ||
max_mem = ( | ||
hook.max_mem | ||
) # the amount of memory in bytes needed for the method according to memoryhook | ||
|
||
# now we estimate how much of the total memory required for this data | ||
(estimated_memory_bytes, subtract_bytes) = _calc_memory_bytes_LPRec( | ||
(1801, 2560), dtype=np.float32(), **kwargs | ||
) | ||
estimated_memory_mb = round(slices * estimated_memory_bytes / (1024**2), 2) | ||
max_mem -= subtract_bytes | ||
max_mem_mb = round(max_mem / (1024**2), 2) | ||
|
||
# now we compare both memory estimations | ||
difference_mb = abs(estimated_memory_mb - max_mem_mb) | ||
percents_relative_maxmem = round((difference_mb / max_mem_mb) * 100) | ||
# the estimated_memory_mb should be LARGER or EQUAL to max_mem_mb | ||
# the resulting percent value should not deviate from max_mem on more than 20% | ||
assert estimated_memory_mb >= max_mem_mb | ||
assert percents_relative_maxmem <= 20 | ||
|
||
|
||
@pytest.mark.cupy | ||
|
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.
On
ws448
, on thefourier
branch, two FBP memory hook tests now fail for me:Whereas on
main
, all FBP memory hook tests were passing: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.
yes, as we discussed, this multiplier is speculative as I had to increase it for larger data during benchmarks. Let us start with the memory estimator for it in a separate branch.
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.
Ok, I'm happy for the FBP memory estimation to be dealt with in a separate PR. If so, shall we just leave the FBP memory estimation alone in this PR and only have LPRec changes here?
Ie, shall we get rid of the FBP memory estimation changes here and put them in the PR for FBP memory estimation, since the changes to FBP memory estimation here are "incomplete" anyway?
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.
yep, sounds good. We should remove the FBP part. Sorry I thought it would be a quick fix so I put it here.
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.
No problem, this time I remembered to run the tests before reviewing so it was easy enough to catch, compared to the past times where I forgot to run the tests... 😅