NEW: Add multi-device to lstsq_grad method

AdvancedPhotonSource / tike

Repository for ptychography software

http://tike.readthedocs.io

Other

29 stars 15 forks source link

NEW: Add multi-device to lstsq_grad method #124

Closed carterbox closed 3 years ago

carterbox commented 3 years ago

Purpose

Refactor the lstsq_grad method to use multiple-devices.

Approach

Breaking the lstsq_grad method into more functions and use the threadpool to execute sections in parallel.

Pre-Merge Checklists

Submitter

[x] Write a helpfully descriptive pull request title.
[x] Organize changes into logically grouped commits with descriptive commit messages.
[x] Document all new functions.
[ ] Write tests for new functions or explain why they are not needed.
[ ] Build the documentation successfully
[x] Use yapf to format python code.

Reviewer

[ ] Actually read all of the code.
[ ] Run the new code yourself.
[ ] Write a summary of the changes as you understand them.
[ ] Thank the submitter.

pep8speaks commented 3 years ago

Hello @carterbox! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-03-01 23:45:12 UTC

carterbox commented 3 years ago

@xiaodong-yu, I think that the eigen probe updates are as concise as I can get them, so you can start thinking about how to synchronize those updates across devices.

Maybe we can take a shortcut and just average the eigen probes from each device after each update? I don't know. We'll have to run some tests.

xiaodong-yu commented 3 years ago

@xiaodong-yu, I think that the eigen probe updates are as concise as I can get them, so you can start thinking about how to synchronize those updates across devices.

Maybe we can take a shortcut and just average the eigen probes from each device after each update? I don't know. We'll have to run some tests.

@carterbox, the eigen_probe update is parallelized on multiple devices. Please have a review. The autocheck on multi-GPU seems to fail due to lacking GPUs, please have a look at it. Tests on local machines all succeed.

carterbox commented 3 years ago

@xiaodong-yu Please take a look at this diff https://github.com/tomography/tike/pull/124/commits/ee69efd843f94153678f6d100f187b105415bff8. I think it is clearer to pass the index as an argument instead of doing list comprehension. Do you agree?

xiaodong-yu commented 3 years ago

@xiaodong-yu Please take a look at this diff ee69efd. I think it is clearer to pass the index as an argument instead of doing list comprehension. Do you agree?

As long as the output is correct, I agree with this change.

carterbox commented 3 years ago

I'm testing this branch on a real dataset that needs eigen probes right now. I'll know by tomorrow if everything is working correctly.

carterbox commented 3 years ago

Something has changed the reconstruction quality, but I'll need to investigate whether it is caused by this PR or a previous one.

carterbox commented 3 years ago

The difference in behavior I was observing was due to some unrelated changes from a previous PR. I have patched this and the upstream branch.

carterbox commented 3 years ago

@xiaodong-yu, whenever you approve, merge this PR.