bug in advec_52 and advec_5th

[fjansson]

There is a typo in advec_52.f90 and advec_5th.f90, in the advecv routines.

-   -sign(1.,(v0(i,j+1,k)+v0(i,j,k)))*(v0(i,j+1,k)+v0(i,j+1,k))/60.&  
+   -sign(1.,(v0(i,j+1,k)+v0(i,j,k)))*(v0(i,j+1,k)+v0(i,j,k))/60.&  

The sign(1,x)*x construction is for calculating abs(x). In this particular place, the final term has j+1 where it should be j. This line is present twice in each of the files advec_52.f90 and advec_5th.f90. In advec_5th, only the top and bottom boundaries are affected, not the bulk.

I plan to replace the sign() constructions by abs() for readability. I will patch this expression first as a separate commit, as documentation of why the results change.

[Chiil]

Yikes, that must have been there for a very long time. I principle you should be able to catch all of these errors with a random noise advection test, as it has to close at machine precision, and this one obviously will not. I recall that in the time that we made this routine, that there were performance issues with the abs function, but I am not sure whether that is still valid.

…

On 6 Aug 2018, at 13:02, Fredrik Jansson ***@***.***> wrote: There is a typo in advec_52.f90 and advec_5th.f90, in the advecv routines. - -sign(1.,(v0(i,j+1,k)+v0(i,j,k)))*(v0(i,j+1,k)+v0(i,j+1,k))/60.& + -sign(1.,(v0(i,j+1,k)+v0(i,j,k)))*(v0(i,j+1,k)+v0(i,j,k))/60.& The sign(1,x)*x construction is for calculating abs(x). In this particular place, the final term has j+1 where it should be j. This line is present twice in each of the files advec_52.f90 and advec_5th.f90. In advec_5th, only the top and bottom boundaries are affected, not the bulk. I plan to replace the sign() constructions by abs() for readability. I will patch this expression first as a separate commit, as documentation of why the results change. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#37>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGu6fuNcWzvtLX_YoJ1UexAEcOrwKT6ks5uOCIsgaJpZM4VwL3i>.

[fjansson]

Indeed abs() seems slower for advec_5th. For advec_52, I couldn't measure any significant difference.
I find this surprising, but I will leave the sign construction as it is for now. I get exactly identical output with abs() and the sign construction, after fixing the typo above.
I'll make a pull request for inclusion in 4.2 soon.

[fjansson]

Fix merged in version 4.2.