Fix a mistake in vPQRdot calculation

[mathieuflament]

Related to the issue #1034, here is a proposal to fix the erroneous relationship between vPQRdot and vPQRidot in FGAccelerations::CalculatePQRdot() and FGAccelerations::SetHoldDown()

[seanmcleod]

We'll need to update the orbit check case since it's currently failing based on the expected PQR-dot values which are now different enough.

======================================================================
FAIL: testOrbitCheckCase (__main__.TestOrbitCheckCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/jsbsim/jsbsim/tests/RunCheckCases.py", line 55, in testOrbitCheckCase
    self.assertEqual(len(diff), 0, msg='\n'+diff.to_string())
AssertionError: 3 != 0 : 
                  Time     delta  ref value     value
P dot (deg/s^2)  10000  0.000632   0.000316 -0.000316
Q dot (deg/s^2)   7000  0.002207   1.135944  1.138151
R dot (deg/s^2)   3000  0.002189   1.307908  1.310097

[bcoconni]

Indeed this PR should include an update of the file check_cases/orbit/logged_data/BallOut.csv.

[mathieuflament]

I've just added a commit to this PR, that updates the reference output file for the orbit check case. Only the columns related to vPQRdot have been modified. The test RunCheckCases should run fine now.

[seanmcleod]

I've just approved the build and test run.

[jonsberndt]

So, the check case paper and files are here: https://nescacademy.nasa.gov/flightsim We should be comparing against the set of data here and not against ourselves – is that what we are currently doing? It’s been a while since I looked at this. I can take a look in the coming week or so.

[seanmcleod]

@jonsberndt the JSBSim check cases pre-date the NASA check case paper. I know there was mention years ago of making use of the NASA check cases, but no code was committed to do that I'm pretty sure.

Ideally we'd do both. There are a bunch of check cases we could potentially have that don't exist in the NASA set.

[mathieuflament]

@jonsberndt Clearly, my update of the reference output file BallsOut.csv was not inattended to crosscheck JSBSim results with another simulation software. In my mind, this check case was more likely a non-regression test. Here are some details about how I made the BallsOut.csv update:

• [Step 1] I built the master branch and generate a reference BallsOut.csv file by running the tests. On both Linux/GCC and Windows/MSVC platforms, the results are very close to the BallsOut.csv file archived on the Git repository, but not stricly equal (something between 1e-12 and 1e-9 discrepancy, far below the test tolerance)
• [Step 2] I built JSBSim including the fix, re-run the test and compare with the BallsOut.csv generated at Step 1. I checked, on both Linux/GCC and Windows/MSVC platforms, that the fix leaves all variables stricly unmodified (except, of course, for vPQRdot).
• I built BallsOut.csv file for the PR as a copy of master branch version, with only an update on the PQRdot-related columns. I considered that it was not relevant to modify the rest of data, because the small discrepancies I observed on Step 1 have certainly nothing to do with the fix, but are probably related to the building/running platform.

[bcoconni]

We should be comparing against the set of data here and not against ourselves

@jonsberndt We don't need to compare against anyone. Maths tell that we are wrong: the derivative of increasing functions must be positive (and negative for decreasing functions). As @mathieuflament showed in issue #1034, this simple rule is not met with the current code:

To confirm @mathieuflament analysis, I have just pushed a new commit 7329fbc in master that adds a new test TestPQRdot.py which fails because the # vPQRdot is wrong.

[jonsberndt]

We should be comparing against the set of data here and not against ourselves

@jonsberndt We don't need to compare against anyone. Maths tell that we are wrong: the derivative of increasing functions must be positive (and negative for decreasing functions). As @mathieuflament showed in issue #1034, this simple rule is not met with the current code:

@bcoconni - all true. I should have been more clear. All that I meant was that it would be good to get the atmospheric 6-DoF simulation check cases into the testing reportoire - I was referring to the check cases I mentioned above. I should have put this in a separate thread, but I meant that it would be better to compare against known good sim data rather then only comparing to our own past data - though I can't say that I know a lot about how you guys have set up the regression testing. I had set up JSBSim some years ago to do the automatic comparison against this test case data with very close matching. If PQRdot had been included in the check case data we would have seen this before. I'm also not suggesting that someone else do this. I may even have the check case scripts for JSBSim still around somewhere on my hard drive. If I can find it, I can demonstrate that and propose a way to get that into the testing setup.

[bcoconni]

All that I meant was that it would be good to get the atmospheric 6-DoF simulation check cases into the testing reportoire - I was referring to the check cases I mentioned above.

@jonsberndt Yes, sorry I misunderstood you.

I may even have the check case scripts for JSBSim still around somewhere on my hard drive. If I can find it, I can demonstrate that and propose a way to get that into the testing setup.

That'd be awesome ! Definitely a subject that deserves its own issue/discussion 😄

[seanmcleod]

That'd be awesome ! Definitely a subject that deserves its own issue/discussion

Yep, I've been taking a look at our current check cases and the test framework we have for running them and looking at the NASA check cases again. Will create a separate discussion topic for that, especially since I have a couple of questions already 😉