search

spacetelescope / poppy

Physical Optics Propagation in Python
https://poppy-optics.readthedocs.io
BSD 3-Clause "New" or "Revised" License
216 stars 72 forks source link

cupy support is not quite there #614

Closed JTBakerAO closed 7 months ago

JTBakerAO commented 7 months ago

when i try to speed up poppy with cupy, i need to convert numpy arrays to cupy arrays first, then convert the answers back to numpy arrays to continue, say, plotting with matplotlib, etc. Some poppy plots simply fail using cupy because they don't take care of this when they use probably matplotlib wf.display(). This needs to be addressed, if possible. My fix has been to edit accel_math.py to fail on detecting cupy, then everything runs fine, except there's no cuda acceleration benefit. ):

mperrin commented 7 months ago

Hi @JTBakerAO, and welcome. I see this is your first time ever filing a GitHub issue, so let me give you some suggestions as hopefully constructive feedback. I might also suggest you see this helpful post about open source software; see their "For the User" section.

First, when filing an issue with a bug report, it's helpful to include specific details, such as example code for what you're doing that results in the bug or will trigger the issue. Concrete examples help to make it efficient for the team to be able to investigate and reproduce the issue.

Secondly, please try to keep in mind with open source software that often this work is being done gratis or pro bono, on a best-effort basis. Using dismissive phrases like "not quite there" or demanding phrases like "needs to be addressed" can come across as too demanding, for software that to be honest you're not paying for in any way. Common courtesies like "please" and "thank you" can help indicate respect and appreciation for the real human beings on the other side of the internet, who ultimately you're asking to do work as a favor to you for free...

Now, that said, let's try again. Can you provide please some specific, concrete examples of what you're trying to do that's running into trouble? Runnable code examples would be much appreciated. With that, it might be possible to take a look at this, at some point in the next while, possibly, time permitting from actual job priorities.

That said I do want to make it clear that in general, the cupy code is not intended to interact with displays much at all during the calculations. If you're optimizing for speed, one generally does not want optional graphical displays of wavefronts happening during the calculation; that slows things down. Added overhead for more transfers of data between GPU and CPY memory, and added overhead for the graphical displays. So for our use cases doing high performance calculations on GPUs, the intermediate display code paths are in general not being used. I know for a fact all the optical calculations themselves run fine with cupy; but I'm not surprised that some of the display code paths have not been fully tested for compatibility with GPUs. This is not a high priority for us, though I agree it would be nice for it to have graceful error handling or perhaps raise an exception that directly informs the user that such is not supported.

Cheers, and thanks for reaching out, and congrats on learning how to file a GitHub issue - welcome to the open source community, I hope.

JTBakerAO commented 7 months ago

I appreciate your comments and I mean no disrespect--quite the contrary. I have since found the note "Addition of CuPy as an Accelerated Computing Option #499", which cleared up much confusion. I had installed CuPy for another purpose, but it "broke" applications that I had coded previously using PopPy. Now, later, having need of more speed in Fresnel propagation calculations, I endeavored to sort out the issues. You are correct, CuPy enhancements work fine, internally, especially when using all PopPy objects to set up optical scenarios. Functions like wf.display() will fail, as stated in note #499, which don't have the array conversion built-in to feed the internal graphics functions used. Instead, extracting the data using complex_efield=cp.asnumpy(wf.wavefront), etc., etc., allows me to utilize other calculations and plt.imshow() to work without incident. The issues I was having were related to using externally generated data in the form of numpy arrays being fed into functions when CuPy arrays were expected, whereas numpy arrays worked fine when CuPy was not installed on the system. I appreciate all of the efforts of the builders of PopPy as their algorithms are powerful, clear, stable and hence useful. I feel it has been worth the investment of my time to learn enough to fully utilize many of its features, and I encourage others daily to try it without reservation.