Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.28%. Comparing base (5988bb4) to head (52ae398).
⚠️ Report is 4 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1377      +/-   ##
==========================================
+ Coverage   25.24%   25.28%   +0.04%     
==========================================
  Files         169      169              
  Lines       18549    18569      +20     
==========================================
+ Hits         4682     4696      +14     
- Misses      13867    13873       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Not sure to understand why this opportunity is not taken to replace the eccentricity from the system code by the eccentricity from the C++ code ?

        <property caption="Orbital perigee (miles): "> guidance/executive/periapsis </property>
-        <property caption="Orbital eccentricity:    "> guidance/executive/eccentricity </property>
+        <property caption="Orbital eccentricity:    "> orbital/eccentricity </property>
        <property caption="Altitude AGL (ft):       "> position/h-agl-ft </property>

Also we could go one step further and replace the perigee and apogee computations in the system code by the C++ implementation ?

    <fcs_function name="guidance/executive/apoapsis">
      <function>
        <quotient>
          <difference>
-            <product>
-              <property>guidance/executive/semimajor-axis-length-ft</property>
-              <sum>
-                <value>1</value>
-                <property>guidance/executive/eccentricity</property>
-              </sum>
-            </product>
+            <property>orbital/apoapsis-radius-ft</property>
            <property>inertial/sea-level-radius_ft</property>
          </difference>
          <value>5280</value>
        </quotient>
      </function>
    </fcs_function>

    <fcs_function name="guidance/executive/periapsis">
      <function>
        <quotient>
          <difference>
-            <product>
-              <property>guidance/executive/semimajor-axis-length-ft</property>
-              <difference>
-                <value>1</value>
-                <property>guidance/executive/eccentricity</property>
-              </difference>
-            </product>
+            <property>orbital/periapsis-radius-ft</property>
            <property>inertial/sea-level-radius_ft</property>
          </difference>
          <value>5280</value>
        </quotient>
      </function>
    </fcs_function>

Ping ! Any comments about my suggestion ?

I just tried your suggestion 1 and then added suggestion 2. These all work also. The outputs are just slightly different from the original (due to rounding error) although it grows over time. In both cases O3 does equal Ofast, so that problem is not there. If we change scripts/J2461.xml then we should also change scripts/J2460.xml which has the same property.

Here are the outputs from J2461.xml.

O3-J2461-bc.xml.txt
O3-J2461-bc2.xml.txt
Ofast-J2461-bc.xml.txt
Ofast-J2461-bc2.xml.txt

This is the difference in O3 when replacing the apogee/perigee computations (within FP rounding errors).

I just tried your suggestion 1 and then added suggestion 2. These all work also.

Great!

The outputs are just slightly different from the original (due to rounding error) although it grows over time.

This is not surprising since, as I have explained in #834 (comment), I think there is an error in the computation of guidance/executive/eccentricity.

Now I think we can safely remove the computations of guidance/executive/eccentricity and guidance/executive/semimajor-axis-length-ft (i.e. the 2 <fcs_function> elements listed below) since they are no longer needed.

And after that, this PR will be ready for merging, unless someone has some other comments.

Is everyone happy with the changes ? @seanmcleod70 @agodemar ?
Because I am 😃

@bcoconni it's OK for me

Looks good to me.