search

jgliss / pyplis

Python toolbox for the analysis of UV SO2 camera data
GNU General Public License v3.0
7 stars 5 forks source link

DilutionCorr.det_topo_dists_line() throws linspace error in MeasGeometry.get_viewing_directions_line() #33

Open twVolc opened 3 years ago

twVolc commented 3 years ago

Versions which may be useful for this issue:

Python 3.8.2
pyplis 1.4.4
numpy 1.18.1

I am getting an error when trying to use the DilutionCorr class with lines I am extracting manually from the on-/off-band images using click events on a matplotlib figure of the images. Having initiated the DilutionCorr object with two lines, I then call det_topo_dists_line(line_id) on the object. The end of the traceback is:

Traceback (most recent call last):
  File "C:\Users\tw9616\Anaconda3\envs\py38\lib\site-packages\pyplis\geometry.py", line 777, in get_viewing_directions_line
    x = linspace(x0, x1, l)
  File "<__array_function__ internals>", line 5, in linspace
  File "C:\Users\tw9616\Anaconda3\envs\py38\lib\site-packages\numpy\core\function_base.py", line 119, in linspace
    raise TypeError(
TypeError: object of type <class 'numpy.float64'> cannot be safely interpreted as an integer.

Thinking that this may be an issue with the line coordinates x0, x1, I have tried casting my line coordinates to int before generating the the LineOnImage objects to pass to DilutionCorr, but this didn't solve the problem. It seems that the issue is line 776 in geometry.py: l = sqrt(delx ** 2 + dely ** 2). This produces a float, which is throwing an error on the next line when using l in linspace. It might be that I am using these functions incorrectly, or I have set something up wrong to start with. I have found that casting l to an int solves the problem and the function runs to completion: l = int(np.round(sqrt(delx ** 2 + dely ** 2)))

I have tried to confirm this issue by running ex11_signal_dilution.py from pyplis example scripts and I do get the same error, which gives me some confidence that I haven't made a stupid mistake somewhere. Following my above edit the example script appears to run correctly. Perhaps this version of numpy has slightly different handling of int/float in linspace?

jgliss commented 3 years ago

@twVolc thanks for this, yeah seems like this is due to int/float and should be an easy fix. Will pick it up for the next release! If you have fixed it already (and perhaps the other issues you found) feel free to create a PR via a fork. Cheers

twVolc commented 3 years ago

Hi @jgliss Thanks for the response and I really appreciate all of your work to provide this package! I haven't ever collaborated on github projects (but really should learn how to...), so I may make a mess of trying create a PR, but I can have a look into it and see what state my current pyplis module is in (I can't remember exactly what changes I have made to it).

jgliss commented 3 years ago

Hey @twVolc thanks a lot for offering to contribute. If you create a PR from your fork, we can make it a draft and take it from there. With an open PR GitHub is really helpful there to revise changes from the diff.

twVolc commented 3 years ago

Hi @jgliss I very stupidly made these changes on a conda install of pyplis locally, rather than on a forked version. I've just forked pyplis now and will try to get to grips with things so hopefully I can be a bit more helpful in the future! I'll try to integrate the fixes to the small bugs in this at some point and create a PR.

jgliss commented 3 years ago

@twVolc no problem and thanks for helping! I am very happy for any contributions / feedback from your side. You can also create a draft PR and just push to that branch in your fork, whenever you have time and ping me in the PR if you want me to review any of your updates. Cheers, Jonas