search

mperrin / poppy

Physical Optics Propagation in Python
BSD 3-Clause "New" or "Revised" License
177 stars 41 forks source link

python3 Integer division in poppy/utils.py #235

Closed exowanderer closed 7 years ago

exowanderer commented 7 years ago

I was using poppy.utils.pad_to_size for another code and kept receiving a ValueError:

Traceback (most recent call last):
  File "$HOME/anaconda3/lib/python3.6/site-packages/pynrc/speckle_noise.py", line 110, in opd_seg
    imsub = pad_to_size(imsub, [npix_max,npix_max])
  File "$HOME/anaconda3/lib/python3.6/site-packages/poppy/utils.py", line 951, in pad_to_size
    padded[n0:n1, m0:m1] = array
ValueError: could not broadcast input array from shape (225,238) into shape (226,238)

Tracking down the problem I found that on line 943 and line 944, the function poppy.utils.pad_to_size was generating the pixel offsets using implicit float division (python3) -- of course this used to be implicit integer division for python2.

After changing line 943 & 944 to:

n0 = (outsize0 - array.shape[0]) // 2  # pixel offset for the inner array
m0 = (outsize1 - array.shape[1]) // 2  # pixel offset in second dimension

the problem went away.

I went ahead and changed all / 2 to // 2, just to be safe.

I'm submitting this as an issue instead of a PR because it's something that is an easy fix, but probably needs to be fixed in a lot of places.

Note: the other code was github.com/eas342/tso_sim, which is a wrapper for github.com/jarronl/pynrc. I am leaving this here for future search engines connect the pynrc usage errors to this poppy issue.

mperrin commented 7 years ago

@exowanderer Thanks. I agree this needs to be fixed for that specific function, but I don't think it's necessary to change all / 2 to // 2 as you suggest - in many cases in the Fourier optics calculations etc we do want floating point division. I'd have to go case-by-case for each division to check which was intended rather than using a blanket search and replace, and I don't think that's warranted right now.

I made the changes to pad_to_size only, and expanded the unit test for that function to include more tests cases to handle all the possible even and odd parities. The poppy test suite all passes with that change, so I'm going to leave it at that minimal set of changes for now.

exowanderer commented 7 years ago

It's definitely a case by case issue. I went through all of the division by 2 markers in poppy/utils.py and found that whenever integer division was desired, the code read " / 2"; but if float division was desired, then the code read " / 2.0". So I changed all of the " / 2" to " // 2", but kept all of the " / 2.0" intact.

Note that I only looked at that one file.

The rest is up to you :)