Fix NaN results

[kivarsen]

I was getting NaN results from some calls in WWA. I updated the vvd method in WWATest to look for NaN, and re-running reported the following failed tests:

wwaAtic13 failed: rc want 2.7101265045317167 got NaN (1/NaN)
wwaAtic13 failed: dc want 0.17406325376270346 got NaN (1/NaN)
wwaAticq failed: rc want 2.7101265045317167 got NaN (1/NaN)
wwaAticq failed: dc want 0.17406325376270346 got NaN (1/NaN)
wwaAtciqn failed: rc want 2.7099995750330272 got NaN (1/NaN)
wwaAtciqn failed: dc want 0.17399996563164699 got NaN (1/NaN)
wwaAtoc13 failed: R/rc want 2.7099567446607318 got NaN (1/NaN)
wwaAtoc13 failed: R/dc want 0.17416965008964389 got NaN (1/NaN)
wwaAtoc13 failed: H/rc want 2.7099567446607318 got NaN (1/NaN)
wwaAtoc13 failed: H/dc want 0.17416965008964389 got NaN (1/NaN)
wwaAtoc13 failed: A/rc want 2.7099567446607318 got NaN (1/NaN)
wwaAtoc13 failed: A/dc want 0.17416965008964389 got NaN (1/NaN)

These were due to uninitialized arrays in wwaASTROM. In most places throughout the code, a new wwaASTROM instance is followed by these lines:

astrom.eb = new double[3];
astrom.eh = new double[3];
astrom.bpn = new double[3, 3];

I added these to a few places where they were missing, and now all tests pass.

[abrudana]

The test was passed successfully without these changes!

[kivarsen]

Thanks for the reply (and for your work in putting together this library)! You are right that when you run WWATest without modification it reports that all tests have passed. However, the functions atic13 and atoc13 are actually returning NaN, and WWATest is not marking this as a failure. These errors go undetected because vvd() will always pass if given NaN as a value. For example, in Program.t_atoc13(), the line:

vvd(rc, 2.709956744660731630, 1e-12, "wwaAtoc13", "R/rc", ref status);

will report success even though rc holds the value NaN instead of a number close to 2.70995...

The first fix was to make vvd() explicitly check for NaN. After doing this, WWATest reported the failures I included in my first comment, which could be fixed by initializing the arrays in the wwaASTROM structure.

Just to dig into it a bit more: the NaN results were specifically due to the bpn array being null. In apco.cs, the last step is to copy the computed matrix into bpn using:

wwaCr(r, astrom.bpn);

For the problematic functions that didn't allocate astrom.bpn, wwaCr would be handed a null value.

In wwaCr, there is actually code that tries to check if the argument is null and allocate the array:

public static void wwaCr(double[,] r, double[,] c)
{
if (c == null)
c = new double[3, 3];
Array.Copy(r, c, r.Length);
return;
}

However, this would only work if the second argument was "ref double[,] c". As it is written, you can change the contents of the array c points to from within the function, but you can't assign a new array and have it propagate back to the caller.

I hope that clarifies the behavior I was trying to fix. Please let me know if there is anything else I can do to help!

[abrudana]

Hi Kivarsen,
Thank you for your clarification!
The [program.cs] file in my opinion should not be modified!
I would like to keep it as the original one.
What do you think?

[kivarsen]

Hi Attila,

Sorry for taking so long to get back to you!

Are you concerned about Program.cs becoming too different from the original t_sofa_c.c, and being less trustworthy as a test?

My feeling is that when a problem is discovered (such as wwaAtoc13 or wwaAtic13 returning NaN for all inputs), the test suite should first be updated to catch that problem and produce a failed test. Then the problem should be corrected so that the tests pass.

The changes to Program.cs come down to two things:

1. vvd() now checks for NaN and fails the test if detected. The purpose of vvd() is to make sure that val == valok to within some tolerance. If you are trying to validate a result like this:

vvd(rc, 2.709999575033027333, 1e-12, "wwaAtciqn", "rc", ref status);

and rc is NaN, then I can't think of any reason why this shouldn't fail.

I'd argue that t_sofa_c.c should also check for NaN. However, in this case the C implementation doesn't actually produce NaN results in the way that the C# implementation does. In C, the arrays in the ASTROM struct are automatically allocated inline with the rest of the struct, so there is no way for the .bpn member to be null. On the other hand, in C# these members are references to null until the arrays are specifically allocated, and if they are not allocated then the results of Atic/Atoc end up being NaN.

2. In t_aticqn and t_aticqn, I add full initialization for the ASTROM struct before calling wwaAticq or wwaAticqn. As currently written, I think performing this initialization is just a prerequisite for calling the function. Again, thinking about translating from C to C#, the fully-allocated struct you get from the single line:

iauASTROM astrom;

is effectively accomplished in C# by many lines:

WWA.wwaASTROM astrom = new WWA.wwaASTROM();
astrom.eb = new double[3];
astrom.eh = new double[3];
astrom.bpn = new double[3, 3];

So by changing Program.cs to add these, I think it more closely reflects the semantics of the original t_sofa_c.c code.

It would be nice if this extra initialization could be handled in a constructor for wwaASTROM, but this doesn't seem to be something you can easily do with C# structs. If you really wanted to get creation down to one line, I guess you could create a function like wwaASTROMCreator.Create() that creates and returns a fully initialized ASTROM structure, but I avoided doing this because it would involve updating the code in many more places (and still require changing Program.cs).

Thanks again for taking a look.

[abrudana]

Hi Kevin, Thank you for the clarification. Yeah, you are right! Thank you for your job! I am going to commit your changes. Have a nice day! Attila From: kivarsen <notifications@github.com> Sent: Friday, June 5, 2020 11:51 PM To: abrudana/wwa <wwa@noreply.github.com> Cc: Attila Abrudán <abrudana@gmail.com>; State change <state_change@noreply.github.com> Subject: Re: [abrudana/wwa] Fix NaN results (#7) Hi Attila, Sorry for taking so long to get back to you! Are you concerned about Program.cs becoming too different from the original t_sofa_c.c, and being less trustworthy as a test? My feeling is that when a problem is discovered (such as wwaAtoc13 or wwaAtic13 returning NaN for all inputs), the test suite should first be updated to catch that problem and produce a failed test. Then the problem should be corrected so that the tests pass. The changes to Program.cs come down to two things: 1. vvd() now checks for NaN and fails the test if detected. The purpose of vvd() is to make sure that val == valok to within some tolerance. If you are trying to validate a result like this: vvd(rc, 2.709999575033027333, 1e-12, "wwaAtciqn", "rc", ref status); and rc is NaN, then I can't think of any reason why this shouldn't fail. I'd argue that t_sofa_c.c should also check for NaN. However, in this case the C implementation doesn't actually produce NaN results in the way that the C# implementation does. In C, the arrays in the ASTROM struct are automatically allocated inline with the rest of the struct, so there is no way for the .bpn member to be null. On the other hand, in C# these members are references to null until the arrays are specifically allocated, and if they are not allocated then the results of Atic/Atoc end up being NaN. 2. In t_aticqn and t_aticqn, I add full initialization for the ASTROM struct before calling wwaAticq or wwaAticqn. As currently written, I think performing this initialization is just a prerequisite for calling the function. Again, thinking about translating from C to C#, the fully-allocated struct you get from the single line: iauASTROM astrom; is effectively accomplished in C# by many lines: WWA.wwaASTROM astrom = new WWA.wwaASTROM(); astrom.eb = new double[3]; astrom.eh = new double[3]; astrom.bpn = new double[3, 3]; So by changing Program.cs to add these, I think it more closely reflects the semantics of the original t_sofa_c.c code. It would be nice if this extra initialization could be handled in a constructor for wwaASTROM, but this doesn't seem to be something you can easily do with C# structs. If you really wanted to get creation down to one line, I guess you could create a function like wwaASTROMCreator.Create() that creates and returns a fully initialized ASTROM structure, but I avoided doing this because it would involve updating the code in many more places (and still require changing Program.cs). Thanks again for taking a look. — You are receiving this because you modified the open/close state. Reply to this email directly, <#7 (comment)> view it on GitHub, or <https://github.com/notifications/unsubscribe-auth/ABTZ7EU7FQYE5W7VAUEXVA3RVFSERANCNFSM4NGMVENQ> unsubscribe. <https://github.com/notifications/beacon/ABTZ7ETAICWSNLDDR2IIGWTRVFSERA5CNFSM4NGMVEN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEYRTMOA.gif>

[kivarsen]

Thank you very much!