atmtools / typhon

Tools for atmospheric research
http://www.radiativetransfer.org/
MIT License
59 stars 32 forks source link

Error in cartposlos2geocentric #82

Closed fedecutraro closed 6 years ago

fedecutraro commented 6 years ago

Bug report

I got an error when I call cartposlos2geocentric function on geodesy.py

Bug summary

I use atmlab for my research but when I try to do the same in Typhon I got the error

Code for reproduction

My script requires many own variables but I got the same error when I call the function with this parameters

#  cartposlos2geocentric(1,1,1,1,1,1)
#
#

Actual outcome

 IndexError                                Traceback (most recent call last)
<ipython-input-2-9f391afb5afc> in <module>()
----> 1 cartposlos2geocentric(1,1,1,1,1,1)

~/anaconda3/lib/python3.6/site-packages/typhon/geodesy.py in cartposlos2geocentric(x, y, z, dx, dy, dz, ppc, lat0, lon0, za0, aa0)
    702 
    703         fix = np.logical_or(np.isnan(aa), ~np.isreal(aa))
--> 704         aa[np.logical_and(fix, dlat >= 0)] = 0
    705         aa[np.logical_and(fix, dlat < 0)] = 180
    706 

Expected outcome

I expect to receive the same result that I've got from atmlab

Version information

I installed Typhon with pip

I don't know if this is the right way to solve the problem but changing aa[np.logical_and(fix, dlat >= 0)] = 0 for aa[np.bool(np.logical_and(fix, dlat >= 0))] = 0 I've got the right result

I hope I've explained the problem properly

Kind regards,

Federico Cutraro

lkluft commented 6 years ago

Hi Federico,

thanks for your bug report!

The docstring says that...

This version is different from the atmlab version by normalizing the los- vector and demanding all or nothing for the optional arguments to work.

Unfortunately, I am not familiar with the differences in detail.

@riclarsson can you help us out with this issue?

Cheers, Lukas

riclarsson commented 6 years ago

Hi Lukas, Federico,

This is apparently an issue with the numpy broadcast_array function that we use. It seems it is not as compatible with the original code as we thought because it creates arrays of 0 size if only presented with scalars. Such arrays cannot be accessed by normal indexing.

I will use a custom broadcasting function instead. I will make it as similar to the part of broadcast_array we use as I can but it is inherently ugly code...

//Richard

Ps. Anyone knows who to report this lack of a feature to the numpy guys? Looking at their code, it is an incredibly easy change to also request a minimum dimension size in broadcast_arrays. (Literally just prepend 1s to shape until it is the right size.)

2018-01-04 17:12 GMT+09:00 Lukas Kluft notifications@github.com:

Hi Federico,

thanks for your bug report!

The docstring says that...

This version is different from the atmlab version by normalizing the los- vector and demanding all or nothing for the optional arguments to work.

Unfortunately, I am not familiar with the differences in detail.

@riclarsson https://github.com/riclarsson can you help us out with this issue?

Cheers, Lukas

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atmtools/typhon/issues/82#issuecomment-355222340, or mute the thread https://github.com/notifications/unsubscribe-auth/ADV4OHaCrMBPWYYbzqlxBBccqg73yMa3ks5tHIgIgaJpZM4RSbHU .

riclarsson commented 6 years ago

The latest commit fix this particular issue. Hopefully id did not introduce any more errors...

lkluft commented 6 years ago

Hi Richard,

thanks for the fix!

numpy is also using GitHub's issue and pull request system. But I think that this is the intended behaviour. If all input arguments are of type float, no broadcasting into arrays should be done.

But I think for typhon it is fine, although it is slightly inconsistent that a call with floats returns one-dimensional ndarrays.

Cheers, Lukas

lkluft commented 6 years ago

@fedecutraro,

the fix to your issue is now merged to the master branch. If you are using the development version of typhon an update of your local working copy (git pull) should fix the problem.

There is also a workaround for the stable version (if you have used pip install typhon directly) by making one of the arguments iterable (tuple, list or np.ndarray):

>>> cartposlos2geocentric([1], 1, 1, 1, 1, 1)
(array([ 1.73205081]), array([ 35.26438968]), array([ 45.]), array([ 0.]), array([ 0.]))

Cheers, Lukas