[PyKEP 2.4-2.6.4] mga_lt_nep: fb_rel_vel is documented in km/s but used directly as m/s bounds

[alebeigit]

Scope

This concerns the legacy pykep.trajopt.mga_lt_nep implementation in
PyKEP 2, from v2.4 through v2.6.4 and the final pykep2 branch.

The class is no longer present in PyKEP 3.

Summary

fb_rel_vel is documented and demonstrated as a value in km/s, but the
constructor stores it without converting it to m/s. get_bounds() then uses
the value directly for the incoming and outgoing intermediate-flyby velocity
component bounds.

Consequently, fb_rel_vel=6 produces component bounds of +/-6 m/s, whereas
the documented API implies +/-6000 m/s.

Inconsistent Documentation And Code

The constructor documentation states that fb_rel_vel is in km/s:

https://github.com/esa/pykep/blob/v2.6.4/pykep/trajopt/_mga_lt_nep.py#L43-L56

The public example notebook independently states:

fb_rel_vel = 6.  # This is in km/s

https://github.com/esa/pykep/blob/v2.6.4/doc/sphinx/examples/ex3.ipynb#L96

The constructor converts the neighboring departure and arrival inputs:

self._vinf_dep = vinf_dep * 1000
self._vinf_arr = vinf_arr * 1000

but stores the flyby value unchanged:

self._fb_rel_vel = fb_rel_vel

https://github.com/esa/pykep/blob/v2.6.4/pykep/trajopt/_mga_lt_nep.py#L64-L68

The unchanged value is then used directly in the component bounds:

https://github.com/esa/pykep/blob/v2.6.4/pykep/trajopt/_mga_lt_nep.py#L78-L98

These decision variables are added directly to planet ephemeris velocities,
which PyKEP documents in SI units, and are passed directly to fb_con():

https://github.com/esa/pykep/blob/v2.6.4/pykep/trajopt/_mga_lt_nep.py#L117-L150

No later km/s-to-m/s conversion is performed.

Exact Commit Where The Conversion Was Lost

PyKEP v2.3 explicitly converted the parameter before constructing the bounds:

fb_rel_vel *= 1000

https://github.com/esa/pykep/blob/v2.3/pykep/trajopt/_mga_lt_nep.py#L80-L92

The change first appears in commit
431ae798f9e478398f1e1137e68c0a8d43762f67 from 22 July 2019:

431ae79

In that commit, fb_rel_vel *= 1000 was removed and the new UDP constructor
stored self._fb_rel_vel = fb_rel_vel instead. The same commit retained the
constructor documentation saying that fb_rel_vel is in km/s.

The missing conversion was subsequently included in release v2.4 and remained
absent through v2.6.4 and the final public pykep2 branch.

Minimal Reproduction

import pykep as pk

udp = pk.trajopt.mga_lt_nep(fb_rel_vel=6.0)
lb, ub = udp.get_bounds()

print("incoming:", lb[6:9], ub[6:9])
print("outgoing:", lb[11:14], ub[11:14])

Actual result:

incoming: [-6.0, -6.0, -6.0] [6.0, 6.0, 6.0]
outgoing: [-6.0, -6.0, -6.0] [6.0, 6.0, 6.0]

Expected result under the documented km/s API:

incoming: [-6000.0, -6000.0, -6000.0] [6000.0, 6000.0, 6000.0]
outgoing: [-6000.0, -6000.0, -6000.0] [6000.0, 6000.0, 6000.0]

Runtime Verification

I reproduced this behavior with:

• PyKEP 2.6.4, conda-forge build py311hf9cf680_5;
• PyGMO 2.19.8;
• Python 3.11.15;
• Windows x86-64;
• genuine compiled PyKEP .pyd modules.

The installed _mga_lt_nep.py exactly matched tag v2.6.4 after newline
normalization.

Suggested Correction

Preserving the documented km/s API would require:

self._fb_rel_vel = fb_rel_vel * 1000.0

A test should verify both incoming and outgoing intermediate-flyby component
bounds.

A separate documentation clarification may also be useful. The code applies
fb_rel_vel independently to each Cartesian component:

-fb_rel_vel <= vx <= +fb_rel_vel
-fb_rel_vel <= vy <= +fb_rel_vel
-fb_rel_vel <= vz <= +fb_rel_vel

It does not enforce sqrt(vx^2 + vy^2 + vz^2) <= fb_rel_vel. Therefore, after
correcting the units, fb_rel_vel=6 km/s would permit a vector such as
(6, 6, 6) km/s, whose speed is sqrt(3) * 6 = 10.39 km/s. If this behavior
is intended, the documentation should call fb_rel_vel a bound on each
Cartesian component rather than a maximum relative speed.

[darioizzo]

old pykep is no longer maintained. closing this.