Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avg.pro with GDL 1.0.3 #1650

Closed
AndyAstronomer opened this issue Oct 17, 2023 · 11 comments
Closed

avg.pro with GDL 1.0.3 #1650

AndyAstronomer opened this issue Oct 17, 2023 · 11 comments
Labels
regression Something that used to work does not work anymore

Comments

@AndyAstronomer
Copy link

My working group have discovered a small issue with GDL 1.0.3 and avg.pro in the NASA astronomy library. It no longer reforms a 3D array into a 1D array when the first two dimensions contain a single element. Here is a simple example:

GDL> a=fltarr(1,1,10) & b=fltarr(10,1,10) & c=fltarr(10,10,10)
GDL> help,avg(avg(a,0),0)
<Expression>    FLOAT     = Array[1, 1, 10]
GDL> help,avg(avg(b,0),0)
<Expression>    FLOAT     = Array[10, 1, 10]
GDL> help,avg(avg(c,0),0)
<Expression>    FLOAT     = Array[10]

In these examples, the first two dimensions should have been averaged out, and only the third dimension should be returned, like in the third example. This was properly addressed in version 1.0.0:

GDL> a=fltarr(1,1,10) & b=fltarr(10,1,10) & c=fltarr(10,10,10)
GDL> help,avg(avg(a,0),0)
<Expression>    FLOAT     = Array[10]
GDL> help,avg(avg(b,0),0)
<Expression>    FLOAT     = Array[10]
GDL> help,avg(avg(c,0),0)
<Expression>    FLOAT     = Array[10]

This leads to many problems with many of our codes as we continue to port from IDL to GDL.

@GillesDuvert GillesDuvert added the regression Something that used to work does not work anymore label Oct 17, 2023
@GillesDuvert
Copy link
Contributor

@AndyAstronomer indeed this was due to an unfortunate optimization. PR #1651 should solve this.
Many thanks for the report before we publish a 1.0.4

@alaingdl
Copy link
Contributor

Hi @AndyAstronomer . Is someone in you team registered to the GDL announces mailing list ?

Thanks for the feedback, this change of dims. is not so easy to understand and test in GDL & IDL. Any idea to write some tests will be welcome !

Since AVG is now mentioned as deprecated, I tested whether MEAN is really equivalent

I found a strange behavior in IDL, then I will send them a bug report :

IDL> b=fltarr(10,1,10)
IDL> tmp=mean(mean(b,di=0), dim=1) & print, tmp
         -NaN
% Program caused arithmetic error: Floating illegal operand

@GillesDuvert @pjb7687 I vote for a 1.0.4 ASAP since we need to have feedback on fast GAUSSFIT from end-users
(my mistake not to have a PR on that important change)

@jtappin
Copy link

jtappin commented Oct 18, 2023

Hi @alaingdl , I don't think there's a problem there. The DIMENSION key starts from 1, so DIMENSION=0 is equivalent to not setting the dimension. Hence you are trying to take the mean over a dimension of a scalar so I'd expect the result to be undefined.

P.S. At least GDL gives an error for taking a dimension=1 mean of a scalar.

@alaingdl
Copy link
Contributor

But I can legitimately call MEAN() with dim=0 ?!

IDL> tmp=mean(dist(5), dim=0)
IDL> help, tmp
TMP             FLOAT     =       1.87436

(as you know dist() gives a 2D array)

@jtappin
Copy link

jtappin commented Oct 18, 2023

My initial comment was wrong, and I corrected it. It's (e.g.)

GDL> c = 3.5
GDL> print, mean(c)
      3.50000
GDL> print, mean(c, dim=1)
% MEAN: Illegal keyword value for DIMENSION
% Execution halted at: $MAIN$          

that gives a NaN in IDL and an error in GDL.

@alaingdl
Copy link
Contributor

I already submitted a bug report to IDL NV, we will see. I found it is not "consistent". I found the message in GDL way better 😄

@AndyAstronomer
Copy link
Author

Hi @alaingdl, no one in the group is on the official list, however, some of my colleagues are based at Observatoire de Paris. If memory serves, there is a GDL announcement email list there, correct?

@alaingdl
Copy link
Contributor

@AndyAstronomer in fact it is a public diffusion list with very few messages, anybody even outside Obs can registered
(link https://sympa.obspm.fr/wws/subscribe/gdl-announces?previous_action=info )

@GillesDuvert
Copy link
Contributor

Well, since we started a discussion 😄 may I ask @AndyAstronomer what difficulties they found (find?) in porting their code from IDL to GDL.
Because, for example, we IMHO made GDL way better by making RHESSI (and SSW) code working (see #425), just because going through a HESSI calibartion procedure, complete and starting from the GUI widgets showed us many real-life examples that were more enligthening than any IDL documentation.

@AndyAstronomer
Copy link
Author

Hi @GillesDuvert. So @alaingdl may remember helping us some years ago on this problem in Paris. We had quite some difficulty getting the call_external function to properly handle the Fortran parts of the code, especially when they were compiled using Intel Fortran. I cannot fully remember the reasons behind the difficulties; something to do with array stacking optimisation. This was solved by compiling with GNU Fortran. Other than that, it took some time as we found that the syntax is stricter in GDL, which is no bad thing So we spent some time editing dummy arguments and calls for a number of the subroutines that make up our code. It mostly works now, except for this new issue.

GillesDuvert added a commit that referenced this issue Oct 19, 2023
@GillesDuvert
Copy link
Contributor

Just merged my patch and closed this issue. Thanks for the report and sorry for the inconvenience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something that used to work does not work anymore
Projects
None yet
Development

No branches or pull requests

4 participants