Refactor/add type hints - #128
Merged
andytorrestb merged 9 commits intoJul 26, 2026
Merged
Conversation
pyrpod/ and its subpackages carry no __init__.py, so mypy derived two module
names for the same file (e.g. fs from the file path and pyrpod.util.io.fs
from an import) and aborted before type checking anything:
pyrpod/util/io/fs.py: error: Source file found twice under different
module names: fs and pyrpod.util.io.fs
Anchor the package base at the repo root (mypy's own suggested resolution (b))
so a single module name is derived per file. No __init__.py files are added and
no import statement changes, so runtime module resolution is untouched. This is
strictly additive to the mypy configuration; ignore_missing_imports and
strict remain as they were.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for pyrpod/util/**, logging_utils, and config_test. Corrections to existing annotations: * util/math/transform.rotation_matrix_from_vectors declared its inputs as np.ndarray, but every caller (JetFiringHistory.print_JFH_param_curve, PlumeStrikeEstimationStudy.calc_time_multiplier) passes plain Python lists such as [1, 0, 0], and the body normalizes with np.asarray. Widened to numpy.typing.ArrayLike, which is what the docstring already documented as array_like of shape (3,). Return narrowed to NDArray[np.float64]. * logging_utils._mark_owned was annotated (Handler) -> Handler, which erased the concrete subclass and made configure_logging pass a plain Handler into LoggingSession(_counter: _LevelCounter). It has always returned the same object it was given, so it is now generic over a Handler-bound TypeVar. This removes a real pre-existing mypy error. Notes: * numpy-stl ships py.typed, so mesh-carrying signatures use stl.mesh.Mesh rather than Any. That also showed BaseStl.from_file/save accept only str, so the STL path parameters are annotated str rather than widened to os.PathLike. * file_print does no NumPy work at runtime, so numpy stays a TYPE_CHECKING import there, behind a __future__ annotations import. * config_test: the twelve-entry thruster-group literal is hoisted to an annotated module constant so the deliberate reliance on configparser stringifying list values is stated in one narrow ignore instead of twelve. The generated example.ini is byte-identical (verified by hash). mypy is clean on these files; tests/rpod/rpod_unit_test_03.py, rpod_unit_test_04.py and tests/logging pass (61 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for Vehicle, VisitingVehicle, TargetVehicle and LogisticsModule. Type definitions introduced (in VisitingVehicle, where the TCF/CCF parsers that produce them live): * ThrusterConfig / ClusterConfig TypedDicts for one parsed thruster or cluster entry. The keys and value shapes are pinned by process_thruster_def/process_cluster_def and by every consumer (['name'][0], ['exit'][0], ['dcm']); tests/rpod/rpod_unit_test_04.py and tests/rpod/rpod_integration_test_08.py build the same shape. Declared total=False solely so the existing key-by-key parser construction keeps type-checking without touching the implementation. 'dcm' accepts nested lists or an ndarray because SweepConfig's cant sweeps overwrite it with np.dot output. Attribute annotations, added only where the inference would otherwise be wrong (bare declarations, so no class attribute is created at runtime): * VisitingVehicle._thruster_id_map: dict[str, str] | None -- first assignment mypy sees is ``= None`` in set_thruster_config. * VisitingVehicle.thruster_metrics: dict[str, Any] | None -- a real Optional: set_thruster_metrics leaves it None when the case has no [tcd] tdf, and PlumeStrikeEstimationStudy plus tests/logging observe that None. Any at the value level because pandas to_dict(orient='records') yields Hashable-keyed records. * LogisticsModule.rcs_groups: dict[str, list[str]] -- annotated to the class contract rather than dict|None, because the ``= None`` written on a missing thruster-grouping file is a dead-end sentinel that no consumer checks. Four narrow ignores were added, each on a pre-existing unguarded-None use that only became visible once the enclosing signature was annotated (mypy skips unannotated bodies). Each carries a comment; none of them changes behavior, and each marks a latent bug rather than hiding one: LogisticsModule rcs_groups=None assignment, the two active_thrusters uses in plot_thruster_group (empty group leaves it None), and the unguarded thruster_metrics lookup in calc_overshoot_v_range. mypy is clean on pyrpod/vehicle. tests/rpod, tests/mdao and tests/mission pass (60 passed, 9 skipped). No new flake8 findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for every module under pyrpod/mission plus pyrpod/orbital/HohmannTransfer, which the mission orbital-transfer engine calls directly. Corrections to existing annotations: * MissionEnvironment.clone was annotated -> "MissionContext"; no such class exists anywhere in the repository. copy.deepcopy(self) returns this class, so the forward reference is now "MissionEnvironment". * MissionEnvironment.get_current_state was annotated Dict[str, np.ndarray], but all four kinematic dataclass fields default to None and nothing in the class populates them, so the mapping is Optional-valued. This removes four pre-existing mypy errors. * OrbitalTransferEngine.add_hohmann_transfer declared leg_id: str = None (implicit Optional, rejected by mypy's default). Now str | None. * HohmannTransfer.__init__ declared leg_id: str = "", but the orbital-transfer engine passes None for an unnamed leg and `leg_id or f"..."` is what supplies the fallback label, so None has always been accepted. Now str | None. HohmannTransfer._compute_transfer was also missing its return type; it returns summary(verbose=False), i.e. dict[str, Any]. Attribute annotations were added, bare so no class attribute is created at runtime, for collaborator state that these WIP submodules read but never define: SixDOFDynamics.vv, ThrusterGrouping.vv/cant, FuelManager.vv/jfh/cant/rotational_maneuvers plus the three thruster-group helpers it calls on self, PostProcessor.vv/flight_plan and the three helpers it calls on self, and FlightEvaluator.flight_plan (Optional: read_flight_plan leaves it None when the case configures no flight plan). Narrow ignores, each commented, all on pre-existing defects that only became visible once the enclosing signature was annotated (mypy skips unannotated bodies) and none of which is safe to fix under an annotation-only change: * flight_eval.calc_flight_performance and orbital_transfer.add_hohmann_transfer are each defined twice; the second definition unconditionally shadows the first. * thruster_grouping.calc_thrust_sum calls np.cos without numpy imported, so that branch raises NameError today. Adding the import would turn a crash into a working calculation, which is a behavior change. * MissionEnvironment.get_thruster_by_id reads self.thruster_data, which is never assigned; get_jfh_segment calls jfh.query, which JetFiringHistory does not implement. * Two flight_plan uses that are Optional by declaration but unguarded. * SixDOFDynamics.calc_trans_performance's bare `return` in the "no thruster grouping" branch: mypy demands an explicit `return None` whenever the declared return type is not plain None (verified with a minimal repro), and the two compile identically, so the source was left alone. The five plotting accumulators in post_processing are annotated Any: they are built as lists and then rebound to the NumPy array made from them, and a list|NDArray union does not survive mypy's loop binder (verified). mypy is clean on pyrpod/mission and pyrpod/orbital. Full pytest: 192 passed, 12 skipped. No new flake8 findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for RarefiedPlumeGasKinetics, CaiImpingement2016, PlumeStrikeCalculator and IsentropicExpansion. Type definitions introduced: * RarefiedPlumeGasKinetics.FloatOrArray (float | NDArray[np.float64]) for the closed-form field expressions, which are pure NumPy ufunc math and are evaluated on meshgrids by CollisionlessGasKinetics._compute_field_integrals and tests/plume/plume_figure_utils.py -- their docstrings already said "float or ndarray". * RarefiedPlumeGasKinetics.Scalar (float | np.floating[Any]) for field-point coordinates. The impingement pipeline builds distance with np.linalg.norm and theta with 3.14 - np.arccos(...), which produce np.floating, not float. * CaiImpingement2016.FloatArray / CoefficientField shorthands, purely to keep the repeated return annotations inside the 88-column limit. * Overloads on get_K_factor / get_M_factor / get_N_factor (scalar Q -> float, array Q -> array). Without them SimplifiedGasKinetics.K_simple/M_simple/ N_simple would carry the union and the Eq. 14-17 ratios built from them could not be typed as float; NumPy's scalar-in/scalar-out semantics make the distinction real. * Overloads on run_parallel_plume_strikes keyed on Literal[True]/[False] for return_meta, which selects the return shape entirely. Corrections and notable choices: * PlumeStrikeCalculator._surface_loads_with_incidence and _parallel_worker_compute had no return type at all; _parallel_worker_compute was implicitly documented as "-> Any". * The unguarded thruster_metrics[t_type] lookup in _compute_plume_strikes_core carries a narrow ignore: the Optional is real (compute_plume_strikes passes getattr(vv, 'thruster_metrics', None)) and the use_kinetics pairing that makes it safe is a caller invariant. * IsentropicExpansion.plot_temp_ratios_vs_radius and plot_number_density_ratios_vs_radius multiply a Python list by a float, which raises TypeError; both are referenced only from commented-out lines in tests/plume/plume_verification_test_01.py. Flagged with narrow ignores, not fixed -- scaling the ratios would be a numerical change. * run_flowfield3d_sanity_checks' kw bundle is annotated dict[str, Any]: it is forwarded with ** into a function whose parameters are not all float. mypy is clean on pyrpod/plume. tests/plume: 78 passed, 3 skipped. No new flake8 findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for JetFiringHistory, PlumeStrikeEstimationStudy and PlumeStudyExport (geometry.py, approach_maneuvers.py, io.py and header.py were already complete). With this commit `python -m mypy pyrpod` passes on the whole package. Corrections to existing annotations: * PlumeStrikeCalculator.run_parallel_plume_strikes was described as returning List[Optional[...]] because results/metas are pre-allocated with None so they can be filled in firing order. Every slot is assigned from a future before the function returns, so the public return type is now the non-Optional list, with the two return statements carrying a narrow ignore that documents the pre-allocation. This is what let PlumeStrikeEstimationStudy.jfh_plume_strikes type-check without ~15 suppressions of its own. * PlumeStrikeEstimationStudy.graph_clusters returns None when no clusters are configured (the comment above the return says so); annotated mesh.Mesh | None. * jfh_plume_strikes returns firing_data keyed by the 1-based firing number as a string, not by int. Attribute annotations (all bare, so no runtime attribute is created): * JetFiringHistory.JFH: list[dict[str, Any]], annotated to the class contract for the same reason as LogisticsModule.rcs_groups -- read_jfh writes None on two failure paths, but only FuelManager.calc_total_delta_mass checks for it while roughly forty sites in this study index JFH unguarded. Two narrow ignores mark the sentinel writes. dict[str, Any] rather than a TypedDict because the records genuinely exist in two shapes: read_jfh parses the time fields as strings with nested-list dcm/xyz, while tests and edit_1d_JFH synthesize steps with NumPy arrays under the same keys. * PlumeStrikeEstimationStudy: jfh/target/vv/viz/case_key/count plus the four thruster-group and fuel helpers it calls on self but neither it nor MissionPlanner defines. * MissionPlanner.cant, because calc_jfh_1d_approach assigns it as a class attribute on MissionPlanner and reads it straight back. Narrow ignores for pre-existing defects, each commented: * graph_jfh_thruster_check builds its frame index with an undefined name `i` on the >= 100 firing branch (flake8 already reports this as F821). * make_test_jfh is declared without self and never called. * Two unguarded thruster_metrics lookups in calc_time_multiplier. * Two active_clusters appends where graph_clusters' None return is excluded by the use_clusters flag rather than by a type-visible check. mypy is clean on all 35 files. Full pytest: 192 passed, 12 skipped. No new flake8 findings (total 1008 -> 960). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Complete parameter and return annotations for SweepConfig and TradeStudy, plus the three callables missed by the earlier subsystem passes: rpod.io.write_jfh and the two integrate.quad closures in RarefiedPlumeGasKinetics. Every function and method under pyrpod/ now has a complete signature (331/331 by AST inventory). SweepConfig's sweep methods are typed against the ThrusterConfig TypedDict introduced with the vehicle subsystem, imported under TYPE_CHECKING because VisitingVehicle imports SweepConfig at runtime; a real import would close the cycle. get_Q_full gains the same scalar/array overload pair as the K/M/N factors, so the Eq. 21 centerline quadrature closure can be typed float -> float rather than carrying the union. rpod.io.write_jfh repeats the _Times/_Rows bound from pyrpod.util.io.file_print, whose printers it forwards to. Narrow ignores for pre-existing defects that only became visible once the enclosing signatures were annotated, each commented: * SweepDecelAngles.cant_decel_thrusters calls self.calculate_DCM(cant) with one argument where the method takes (cant, thruster): TypeError today. * SweepDecelAngles.sweep_decel_thrusters_all loops over an undefined `config` (flake8 also reports F821): NameError today. * TradeStudy.init_trade_study instantiates PlumeStrikeEstimationStudy.RPOD; the module's class is PlumeStrikeEstimationStudy, so this is an AttributeError today. Fixing any of the three would invent behavior, which plume-kit#103 excludes; they are recorded in the deferred observations instead. mypy passes on all 35 files. Full pytest: 192 passed, 12 skipped. No new flake8 findings (1008 -> 960). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six of the eight `if TYPE_CHECKING:` blocks added by the preceding commits
existed only to avoid adding a runtime import edge, not to break a cycle.
They are now plain imports:
mission/SubModule.py -> MissionEnvironment
mission/MissionPlanner.py -> JetFiringHistory, LogisticsModule
mission/post_processing.py -> pandas
mdao/TradeStudy.py -> LogisticsModule, TargetVehicle
rpod/PlumeStrikeEstimationStudy.py -> JetFiringHistory, TargetVehicle
util/io/file_print.py -> numpy, NDArray
In file_print the _Times/_Rows aliases move out of the guard with the import;
they evaluate fine at runtime (verified), so the module no longer depends on
`from __future__ import annotations` to resolve them.
Two blocks are kept because removing them raises ImportError, and each now
carries a comment saying so, with the failure mode spelled out:
* mission/flight_eval.py -> MissionPlanner. MissionPlanner imports
FlightEvaluator at module level; a real import fails in both directions on
a partially initialized module.
* mdao/SweepConfig.py -> VisitingVehicle / LogisticsModule. VisitingVehicle
imports this module at module level, so importing
pyrpod.vehicle.VisitingVehicle first -- the normal order -- fails with
"cannot import name 'VisitingVehicle' from partially initialized module".
LogisticsModule subclasses VisitingVehicle and closes the same loop.
Note this changes runtime import edges, not typing: TYPE_CHECKING is False at
runtime and True for mypy, so the guard never affected what was checked. The
trade is uniformity in exchange for mission/mdao/rpod eagerly importing the
vehicle package, and file_print/post_processing eagerly importing numpy and
pandas, which they do no work with at runtime.
Verification beyond the usual suite, since this commit's whole risk is import
order: each of the 33 importable modules under pyrpod/ was imported FIRST in
a fresh interpreter -- all 33 clean.
mypy: clean on all 35 files. Full pytest: 192 passed, 12 skipped. Annotation
coverage unchanged at 331/331. No new flake8 findings (960, vs 1008 on
master); black 199 files failing as on master with no newly-failing file;
isort 138 vs 140 with none newly failing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No `if TYPE_CHECKING:` block remains anywhere under pyrpod/. The previous commit converted the six that were only avoiding a runtime import edge; the two left were real import cycles, and this removes those too. mdao/SweepConfig.py -> plain imports of LogisticsModule and ThrusterConfig. The cycle only existed because of a dead import: VisitingVehicle carried `from pyrpod.mdao import SweepConfig` at module level while never using the name. Verified unused three ways -- no ast.Name or ast.Attribute reference anywhere in the module, flake8 reports it as F401, and nothing in the repository reaches SweepConfig through pyrpod.vehicle.VisitingVehicle. Deleting that one line breaks the loop, so SweepConfig can now import the vehicle classes for real, and one pre-existing F401 goes away with it. mission/flight_eval.py -> guard deleted, FlightEvaluator.execute's parameter is now `mission_planner: Any`. This cycle is structural rather than accidental: MissionPlanner imports FlightEvaluator at module level and constructs one in its __init__, so naming MissionPlanner in flight_eval -- with or without an import -- closes the loop in both directions. execute() is a `pass` stub, so the precision lost is nil today; a comment records why the class name is not used. If the method is ever implemented, the fix is to invert that dependency, not to reinstate the guard. Verification, since this commit's whole risk is import order: each of the 33 importable modules under pyrpod/ was imported FIRST in a fresh interpreter -- all 33 clean. mypy: clean on all 35 files. Full pytest: 192 passed, 12 skipped. Annotation coverage unchanged at 331/331. flake8 959 findings vs 1008 on master with no new findings; black 199 files failing exactly as on master with no newly-failing file; isort 138 vs 140, none newly failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.