Draft
Conversation
Owner
Author
|
Hey @JonahJ27! Let me know what you think of plan. This is basically a more detailed version of what we discussed. Feel free to edit/comment as we refine and work through the steps. Also, I've added you as a collaborator, so you should have write access to all branches except |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #160 +/- ##
==========================================
- Coverage 91.87% 91.78% -0.10%
==========================================
Files 33 34 +1
Lines 6710 6767 +57
==========================================
+ Hits 6165 6211 +46
- Misses 545 556 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Provide a dynamic dispatch point so that solver subclasses can override how SteadyProblems are retrieved at each time step.
Replace all 11 direct self.steady_problems[] accesses with calls to _get_steady_problem_at so that solver subclasses can override retrieval per time step.
Allow solver subclasses to override per time step bound RingVortex initialization by extracting the loop body into a separate method.
Replace oscillating_sinspaces, oscillating_linspaces, and oscillating_customspaces with scalar single-step equivalents (oscillating_sin_at_time, oscillating_lin_at_time, oscillating_custom_at_time). Enable the core movement classes to evaluate oscillations one time step at a time rather than in batch.
Provide the shared foundation that Movement and its feature variant siblings (FreeFlightMovement, AeroelasticMovement) will extend. CoreMovement requires delta_time and num_steps directly, without the auto-estimation and batch pregeneration that Movement performs.
- Make Movement a subclass of CoreMovement, which now owns the shared slots (_airplane_movements, _operating_point_movement, _delta_time, _num_steps, _max_wake_rows) and derived properties (lcm_period, max_period, min_period, static). Movement retains its own slots for num_cycles, num_chords, max_wake_chords, max_wake_cycles, _airplanes, and _operating_points. - Move _lcm and _lcm_multiple from movement.py to _core.py since they are shared infrastructure. Move their tests from test_movement.py to the new test_core.py module and update import paths in test_movement.py. - Movement.__init__ computes _max_period and _static locally before calling super().__init__() (needed for num_steps and max_wake resolution), then pre-populates the inherited cache slots to avoid redundant recomputation. - Remove TestMovementCaching (4 tests) from test_movement.py. These tested internal cache population timing rather than observable behavior, which is already covered by the correctness tests in TestMovement. - Add a "Public Subclasses of Private Parents" section to TYPE_HINT_AND_DOCSTRING_STYLE.md. When a public class extends a private parent, the convention is inverted: the public child keeps a self-contained docstring listing all methods as its own, and the private parent uses a minimal docstring referencing the public child. This is driven by RTD being purely public API. - Update CoreMovement docstrings to follow the new convention: reference Movement for full documentation, keep brief descriptions for contributors. - Add show-inherited-members to autoapi_options in conf.py so inherited properties appear on Movement's RTD page. Suppress the "Bases:" line for private parents in the custom class.rst template to avoid exposing internal classes on the public docs site. - Update _serialization.py: add _all_slots() helper that walks the MRO to collect inherited slots, replace direct __slots__ access with _all_slots() in both _object_to_dict and _object_from_dict, and bump _FORMAT_VERSION to 2.
- Fix the Jinja2 template error that broke the RTD build. The
reject("match", ...) test does not exist in Jinja2; replace it with a
for loop using the built-in "in" operator to filter private parent
classes from the "Bases:" line.
- Add a source-read hook in conf.py that rewrites docs/*.md paths to
*.md when Sphinx processes CONTRIBUTING.md. The original paths are
correct on GitHub (relative to repo root) but fail in Sphinx (resolved
relative to docs/website/). The hook reads the file directly and
rewrites paths before MyST parses them, so both contexts work without
absolute URLs.
- Replace all pterasoftware.readthedocs.io URLs with the canonical
docs.pterasoftware.com domain in README.md and CONTRIBUTING.md.
Use "inherited-members" instead of "show-inherited-members" in autoapi_options. The latter is not a valid option and was silently ignored, preventing CoreMovement properties from rendering on Movement's RTD page.
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
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.
Description
Extract shared class infrastructure that both
feature/free_flightand PR #156 need, so both features build on the same foundation. This is a backward compatible refactoring with no behavior changes to the existing public API.Both features independently implemented a pattern for providing data to the UVLM solver one time step at a time, defining
CoupledUnsteadyProblemandCoupledUnsteadyRingVortexLatticeMethodSolverwith incompatible interfaces. This PR extracts the shared abstraction: a way for the solver to retrieveSteadyProblemsdynamically per time step instead of from a precomputed tuple. Free flight updates operating points (body state from MuJoCo); aeroelasticity updatesWinggeometry (deformation from loads). A single override point,get_steady_problem_at(step), enables both.Design Principles
Immutable configuration classes are universal. Classes like
OperatingPoint,SteadyProblem,Wing,WingCrossSection,Airplane, andAirfoilare immutable configurations. They describe a static state and are agnostic to simulation type. They do not need feature specific sub-classes. Where an attribute is physically general but only used by certain solvers, it may belong on the configuration class with aNonedefault, following the existingnupattern onOperatingPoint. Attributes specific to a particular coupling approach belong in separate domain context objects passed to the solver. The exact placement of individual attributes (e.g.,g_E,omegas_BP1__E) from the currentCoupledOperatingPointis deferred to the feature PRs.Movement classes use core class extraction. Each movement class has a
generate_*s()method that creates all instances for all time steps at once. This is a special case of a more fundamentalgenerate_*_at_time_step()operation. By extracting the per-step method into a core class (defined in the private_core.pymodule), both the standard class (which loops) and the feature variants (which call per-step on demand) share the same generation logic as siblings, rather than in an awkward parent-child relationship.Feature specific state lives in domain context objects. State specific to a particular coupling approach (dynamics integration, structural response) belongs in purpose built domain context objects passed to the solver, not baked into the configuration classes. Examples (names are preliminary):
FreeFlightDynamicsState: inertia tensor, position/orientation tracking (attribute placement ofomegas_BP1__Eandg_Edeferred to the feature PRs)MuJoCoModel: physics engine interface (already exists on thefree_flightbranch)AeroelasticStructuralModel: spring constants, damping, wing densityClass Hierarchies
Core classes for the movement hierarchy:
Core class pattern for
UnsteadyProblem, standard inheritance for the solver:Why core class for problems but standard inheritance for solvers?
CoreUnsteadyProblemholds the shared slots (delta_time,num_steps,max_wake_rows, the mutable result lists, etc.) without theSteadyProblempre-creation that happens inUnsteadyProblem.__init__. This is necessary because coupled variants cannot createSteadyProblems upfront: free flight does not know operating points in advance (they come from dynamics integration), and aeroelasticity does not know deformed wing geometry in advance (it comes from structural response). Without a core class, feature variants would have to either skipsuper().__init__()and manually set all slots, or call it with dummy data. The solver does not have this problem. It does not pre-create anything problematic in__init__, and the shared computation methods (influence matrices, vortex strength solving, load calculation, wake shedding) are all reusable via normal inheritance. The only override points areget_steady_problem_at(step)andrun(). PR #156 already proves this works with standard inheritance.Naming Consistency
Each feature variant has a complete set of movement classes with a consistent prefix, even when some are thin wrappers around the standard behavior. A
FreeFlightMovementalways takesFreeFlight*Movementchildren, and anAeroelasticMovementalways takesAeroelastic*Movementchildren. For example,FreeFlightWingMovementandFreeFlightWingCrossSectionMovementmay just delegate to the standard generation logic since wing geometry is prescribed in free flight, andAeroelasticOperatingPointMovementmay just delegate to the standard oscillation logic sinceOperatingPointsare prescribed in aeroelasticity.Note on
FreeFlightOperatingPointMovementIn free flight,
OperatingPointsare not prescribed via oscillation parameters. They are dynamically determined by the solver as it integrates rigid body dynamics at each time step.FreeFlightOperatingPointMovementstill fits the movement hierarchy because all movement classes serve the same structural role: they hold a base configuration object and a sequence of per time step snapshots. The difference is only in who populates that sequence.OperatingPointMovementprecomputes all snapshots from prescribed oscillation parameters.FreeFlightOperatingPointMovementstarts with just the baseOperatingPointand provides a mutable list that the solver populates as dynamics integration produces new states. This is consistent with how solver mutable attributes already work elsewhere in the codebase.Note on
OperatingPointThe current
CoupledOperatingPointonfeature/free_flightaddsg_E(gravity) andomegas_BP1__E(angular velocity). Whether these belong on OperatingPoint withNonedefaults or in domain context objects is deferred to the feature PRs. Neither attribute is needed by the shared infrastructure, and the decision is better made with concrete consuming code in hand.OperatingPointalready mixes freestream conditions, position, orientation, external forces, and environmental geometry, so the question of whether it should grow further or be refactored is a broader public API concern that should not block this PR.File Organization
All core classes live in a single private module:
pterasoftware/_core.py. The core classes reference each other across the containment hierarchy (CoreMovementacceptsCoreAirplaneMovements,CoreAirplaneMovementacceptsCoreWingMovements,CoreUnsteadyProblemreferencesCoreMovement, etc.). If they lived in the same files as their public children, each file would need to import underscore-prefixed members from sibling files, violating the codebase's convention (and Python best-practices) against importing private members across modules. Putting them in a single private module avoids this: the classes are public members of the module, and the module's underscore prefix signals "internal to the package." This follows the existing pattern of_functions.py,_transformations.py, and_parameter_validation.py. Concrete children import from the module using relative imports (e.g.,from .. import _coreinmovement.py).Motivation
Both
feature/free_flightand PR #156 independently implemented a pattern for providing data to the UVLM solver one time step at a time. Both defineCoupledUnsteadyProblemandCoupledUnsteadyRingVortexLatticeMethodSolverwith incompatible interfaces. If merged independently, reconciliation will be painful. This PR extracts the shared foundation so both features build on the same base.Relevant Issues
Related to PR #156 (aeroelasticity), it's associated issue #28, as well as the
feature/free_flightbranch, and its associated issue #65.Changes
1. Refactor the parent solver
File:
pterasoftware/unsteady_ring_vortex_lattice_method.pyBackward compatible, no behavior change:
get_steady_problem_at(step)method that returnsself.steady_problems[step]by default. PR [FEATURE] Add structural aeroelasticity #156 already adds this method. Use it as a reference for the approach but re-implement manually rather than cherry-picking, to keep the process simple.self.steady_problems[...]accesses withself.get_steady_problem_at(...)calls. These use varied indexing patterns: literal[step]or[0](5 occurrences),[self._current_step]/[self._current_step - 1]/[self._current_step + 1](4 occurrences),[steady_problem_id - 1]inside an enumeration (1 occurrence), plus 1 enumeration over the entire tuple in_initialize_panel_vortices. PR [FEATURE] Add structural aeroelasticity #156 also implements this refactoring on the base solver.initialize_panel_vortices_at(step)helper (_initialize_panel_vortex(steady_problem, step)in PR [FEATURE] Add structural aeroelasticity #156) from_initialize_panel_vortices()so sub-classes can override per time step behavior.2. Replace batch oscillating functions with single-step helpers
File:
pterasoftware/movements/_functions.pyPrerequisite for step 3. The existing
oscillating_sinspaces(),oscillating_linspaces(), andoscillating_customspaces()functions compute arrays for all time steps at once and do not support evaluating at a single time step.oscillating_sin_at_time(),oscillating_lin_at_time(), andoscillating_custom_at_time()functions that take a singletimeparameter instead ofnum_stepsanddelta_time, and return a scalar.oscillating_sinspaces(),oscillating_linspaces(), andoscillating_customspaces()and replace uses with the new single-step functions.tests/unit/test_movements_functions.pyand fixtures intests/unit/fixtures/movements_functions_fixtures.py, and delete those that are no longer applicable.pterasoftware/_serialization.py.3. Extract core classes into
_core.pyNew file:
pterasoftware/_core.pyCoreMovementwith: parameter storage, validation, and shared properties (static,max_period,min_period,lcm_period,delta_time,num_steps).Movementto extendCoreMovement, adding: batch generation (generate_airplanes(),generate_operating_points()),_airplanesand_operating_pointstuples, complex delta_time optimization, num_steps calculation from num_cycles/num_chords, max_wake_* parameters.CoreOperatingPointMovementand refactorOperatingPointMovementto extend it.CoreOperatingPointMovementshould have a `generate_operating_point_at_time_step() method.CoreAirplaneMovementand refactorAirplaneMovementto extend it. Same pattern as above.CoreAirplaneMovementshould have agenerate_airplane_at_time_step() method.AirplaneMovementadds control flow optimizations on top: the static geometry path callsgenerate_airplane_at_time_step()` for step 0 and deep-copies for the rest, and the LCM periodicity path calls it for one period and deep-copies for remaining periods.CoreWingMovementand refactorWingMovementto extend it.CoreWingMovementshould have a `generate_wing_at_time_step() method.CoreWingCrossSectionMovementand refactorWingCrossSectionMovementto extend it.CoreWingCrossSectionMovementshould have a `generate_wing_cross_section_at_time_step() method.CoreUnsteadyProblemwith the shared slots (delta_time,num_steps,max_wake_rows, the mutable result lists, etc.) withoutSteadyProblempre-creation. RefactorUnsteadyProblemto extend it.FreeFlightMovement,AeroelasticMovement, and their sub-movement classes against theCore*interfaces. These do not need to be functional end to end, just enough to confirm that the method signatures, parameter passing, and property access patterns work for both use cases. Keep them as starting points for the feature branches.4. Register new classes for serialization
File:
pterasoftware/_serialization.py_CLASS_REGISTRY.5. Add tests
Out of Scope
Feature specific classes are out of scope, with one exception: skeleton implementations of feature movement classes are included to validate the
Core*interfaces (see step 3).The following are fully out of scope and will be added by their respective feature PRs after this merges:
FreeFlightUnsteadyProblemFreeFlightSolverFreeFlightDynamicsState(preliminary name)MuJoCoModel(preliminary name)AeroelasticUnsteadyProblemAeroelasticSolverAeroelasticStructuralModel(preliminary name)Dependency Updates
None.
Change Magnitude
Major: Large change that adds significant new functionality, changes existing behavior, or may affect many parts of the codebase.
Checklist (check each item when completed or not applicable)
mainand is up to date with the upstreammainbranch.--in-place --black). See the style guide for type hints and docstrings for more details.pterasoftwarepackage use type hints. See the style guide for type hints and docstrings for more details.testspackage.testspackage.black,codespell, andisortGitHub actions.mypyGitHub action.testsGitHub actions.Additional Verification
__slots__, follow immutability conventions, and are registered for serialization.save()/load()round trips work.