search

manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
131 stars 50 forks source link

adda on gpu and improved dpl #370

Closed golovart closed 3 years ago

golovart commented 3 years ago

Adding an option to run ADDA on GPU (adda_ocl) and to select the GPU index.

dpl: As mentioned in ADDA manual the dipoles need to be "small compared to both any structural length in the scatterer and the wavelength" (Section 4. Applicability of the DDA). For scatterers much smaller than the wavelength they suggest "to use at least 10 dipoles along the smallest dimension", which I tried to implement using the scatterer.bounds. This is not the perfect solution for complex structures, but in that case user should better specify the dipole size himself as max_dpl_size. Maybe some kind of warning to keep an eye on the dipole size for small scatterers should be added in the documentation?

pep8speaks commented 3 years ago

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

Line 100:29: E225 missing whitespace around operator Line 100:31: E701 multiple statements on one line (colon) Line 100:80: E501 line too long (123 > 79 characters) Line 113:39: E701 multiple statements on one line (colon) Line 113:59: E231 missing whitespace after ',' Line 150:80: E501 line too long (100 > 79 characters) Line 171:80: E501 line too long (98 > 79 characters) Line 198:80: E501 line too long (82 > 79 characters)

Comment last updated at 2020-09-28 10:30:46 UTC
barkls commented 3 years ago

Thanks for this PR! I just have a couple minor points before merging in:

1) Have you confirmed that this works both with and without a gpu_id specified?

2) If someone sets use_gpu=True, then the n_cpu parameter is ignored. It would be nice to raise a warning or exception if n_cpu is also set to some other value to prevent confusion. However, it's not a big deal so I can merge without this check if you prefer.

golovart commented 3 years ago
  1. There was a small typo when passing the "gpu_id", now I confirm that it is running on that gpu, which index is passed. Without specifying it adda chooses the default gpu itself.
  2. I added the multi-cpu warning.

Should I correct those warnings above (like too long lines)?

barkls commented 3 years ago

Looks good! Thanks for the update to dipole scaling as well. No need to worry about the overly long lines. Thanks again for the contribution!