ENH: class encoding refactor for large sims #58
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
aasitvora99
requested review from
GabrielBarberini,
Gui-FernandesBR,
Copilot and
phmbressan
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors serialization to a size-aware encoder and attribute collection (InfinityEncoder + collect_attributes), replaces rocketpy_encoder/DiscretizeConfig usage across services, expands flight API schema and renames environment→env, adds fins sweep fields, relaxes some rocket drag types, and changes RocketModel.center_of_mass_without_motor from int to float. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Service as Service (env/flight/motor/rocket)
participant Utils as collect_attributes
participant Encoder as InfinityEncoder
participant Model as Simulation (Env/Rocket/Motor/Flight)
Client->>Service: get_*_simulation()
Service->>Utils: collect_attributes(self.obj, [TargetClasses])
Utils->>Encoder: rocketpy_encoder(obj)
Encoder-->>Utils: encoded JSON/dict (size-aware)
Utils-->>Service: encoded_attributes
Service->>Model: Model(**encoded_attributes)
Model-->>Service: Simulation instance
Service-->>Client: return Simulation
note right of Encoder: down-samples large Function/Flight solutions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the class encoding system for RocketPy simulations to address performance issues in the /simulate route by limiting function samples to 25 points. The changes introduce a new InfinityEncoder that reduces data size and replaces the previous DiscretizeConfig-based approach with a more streamlined collect_attributes function.
Key changes:
- Replace
DiscretizeConfigandrocketpy_encoderwithInfinityEncoderandcollect_attributes - Limit function discretization to 25 samples maximum to improve response times
- Update
FlightSimulationclass hierarchy and add new flight attributes
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/rocket.py | Change drag attributes from typed lists to Any type |
| src/views/flight.py | Major refactor of FlightSimulation class - changed inheritance, added many new attributes, fixed duplicates |
| src/utils.py | Complete rewrite of encoding logic with new InfinityEncoder and collect_attributes function |
| src/services/rocket.py | Update to use new collect_attributes function |
| src/services/motor.py | Update to use new collect_attributes function |
| src/services/flight.py | Update to use new collect_attributes function |
| src/services/environment.py | Update to use new collect_attributes function |
| src/models/sub/aerosurfaces.py | Add new fin attributes sweep_length and sweep_angle |
| src/models/rocket.py | Fix type annotation for center_of_mass_without_motor |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| inclination: Optional[float] = None | ||
| heading: Optional[float] = None |
There was a problem hiding this comment.
Duplicate attribute definitions for inclination and heading. These attributes are already defined on lines 27-28, creating redundant field declarations.
| inclination: Optional[float] = None | |
| heading: Optional[float] = None |
There was a problem hiding this comment.
true, we should remove this
| mutate_self=False, | ||
| ) | ||
| if isinstance(obj, Flight): | ||
| obj._Flight__evaluate_post_process |
There was a problem hiding this comment.
This line accesses a private method but doesn't call it or assign the result. It should either call the method with parentheses obj._Flight__evaluate_post_process() or remove this line if it's not needed.
| obj._Flight__evaluate_post_process | |
| obj._Flight__evaluate_post_process() |
There was a problem hiding this comment.
seems to be a legit statement from copilot
There was a problem hiding this comment.
The method is actually a cached property. It needs to be called at this step to make sure the Flight has its value cached for internal usage. Note: when I pop some of its attributes at the end to reduce size, this property would break if called after, caching it prevents this problem.
| except Exception: | ||
| continue | ||
| return attributes | ||
| return rocketpy_encoder(attributes) |
There was a problem hiding this comment.
This creates infinite recursion since collect_attributes calls rocketpy_encoder at line 121, which returns the result of collect_attributes. The return should likely be just attributes instead.
| return rocketpy_encoder(attributes) | |
| return attributes |
There was a problem hiding this comment.
Not sure I got what copilot is trying to say but I think return here yields something like rocketpy_encoder(rocketpy_encoder(obj).modified())
.modified() being just an illustrative made up noop to emphasize something changed after instantiation.
is that really what we want?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/utils.py (2)
75-75: Fix the missing function call parenthesesThis line accesses the private method but doesn't call it. Add parentheses to actually execute the post-processing.
- obj._Flight__evaluate_post_process + obj._Flight__evaluate_post_process()
200-200: Fix infinite recursion issueThe function returns
rocketpy_encoder(attributes)which will cause infinite recursion sincerocketpy_encodercallscollect_attributesat line 123. The return should be justattributes.- return rocketpy_encoder(attributes) + return attributessrc/views/flight.py (1)
23-31: Remove duplicate field declarationsinclination and heading are defined twice; keep a single definition to avoid confusion and tooling warnings. Also remove the duplicate “Core Flight attributes” header.
- # Core Flight attributes (always present) - # Core Flight attributes (always present) rail_length: Optional[float] = None inclination: Optional[float] = None heading: Optional[float] = None - inclination: Optional[float] = None - heading: Optional[float] = None
🧹 Nitpick comments (4)
src/views/rocket.py (1)
34-35: Restrictpower_off_dragandpower_on_draginsrc/views/rocket.pyto a specific union rather thanAny
Widening these fromOptional[list[tuple[float, float]]]toOptional[Any]removes the compile-time checks that your service and model (which expectList[Tuple[float, float]]) rely on; if you need to accept multiple encodings, use a union type (e.g.Optional[List[Tuple[float, float]] | Dict[str, Any]]) to preserve type safety.src/utils.py (1)
57-95: Consider extracting the magic number 25The value
25appears multiple times in theInfinityEncoder.defaultmethod as the target sample size. Consider extracting this as a class constant for better maintainability.class InfinityEncoder(RocketPyEncoder): + MAX_SAMPLES = 25 + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def default(self, obj): if ( isinstance(obj, Function) and not callable(obj.source) and obj.__dom_dim__ == 1 ): size = len(obj._domain) reduction_factor = 1 - if size > 25: - reduction_factor = size // 25 + if size > self.MAX_SAMPLES: + reduction_factor = size // self.MAX_SAMPLES if reduction_factor > 1: obj = obj.set_discrete( obj.x_array[0], obj.x_array[-1], size // reduction_factor, mutate_self=False, ) if isinstance(obj, Flight): obj._Flight__evaluate_post_process() solution = np.array(obj.solution) size = len(solution) - if size > 25: - reduction_factor = size // 25 + if size > self.MAX_SAMPLES: + reduction_factor = size // self.MAX_SAMPLES reduced_solution = np.zeros( (size // reduction_factor, solution.shape[1]) )src/services/rocket.py (1)
20-21: Drop MotorSimulation from collect_attributes here; it’s a no-op for Rocket objectscollect_attributes’ MotorSimulation branch expects obj.rocket.motor, so passing MotorSimulation with a Rocket object contributes nothing and can mislead future readers. Trim it (and the unused import) for clarity.
-from src.views.motor import MotorSimulation -from src.utils import collect_attributes +from src.utils import collect_attributes @@ - encoded_attributes = collect_attributes( - self.rocket, [RocketSimulation, MotorSimulation] - ) + encoded_attributes = collect_attributes(self.rocket, [RocketSimulation])Also applies to: 111-115
src/views/flight.py (1)
11-16: Fix outdated docstring about base classesClass no longer inherits from RocketSimulation/EnvironmentSimulation; it now inherits ApiBaseView. Update the docstring to match the implementation.
- Inherits from both RocketSimulation and EnvironmentSimulation, and adds flight-specific - attributes. Uses the new rocketpy_encoder which may return different attributes based - on the actual RocketPy Flight object. The model allows extra fields to accommodate - any new attributes that might be encoded. + API-focused view for RocketPy Flight attributes. + Attributes are collected via collect_attributes and encoded by the RocketPy encoder. + Designed to accommodate evolving fields from the underlying simulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/models/rocket.py(1 hunks)src/models/sub/aerosurfaces.py(1 hunks)src/services/environment.py(2 hunks)src/services/flight.py(2 hunks)src/services/motor.py(2 hunks)src/services/rocket.py(2 hunks)src/utils.py(8 hunks)src/views/flight.py(6 hunks)src/views/rocket.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-15T15:12:21.314Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#38
File: lib/routes/motor.py:75-75
Timestamp: 2024-11-15T15:12:21.314Z
Learning: When modifying API route paths in `lib/routes/motor.py`, changing from `/rocketpy/{motor_id}` to `/{motor_id}/rocketpy` is acceptable when no external consumers are affected. It's acceptable to introduce this breaking change if the team has decided to adopt the new approach, provided that the `README` and related documentation are updated accordingly.
Applied to files:
src/services/motor.py
🧬 Code graph analysis (8)
src/services/environment.py (2)
src/utils.py (1)
collect_attributes(116-200)src/views/environment.py (1)
EnvironmentSimulation(13-78)
src/models/rocket.py (1)
src/routes/rocket.py (1)
update_rocket(56-68)
src/services/flight.py (5)
src/views/rocket.py (1)
RocketSimulation(8-59)src/views/motor.py (1)
MotorSimulation(7-73)src/views/environment.py (1)
EnvironmentSimulation(13-78)src/utils.py (1)
collect_attributes(116-200)src/views/flight.py (1)
FlightSimulation(9-140)
src/views/rocket.py (2)
src/views/environment.py (1)
EnvironmentSimulation(13-78)src/views/motor.py (1)
MotorSimulation(7-73)
src/services/motor.py (2)
src/utils.py (1)
collect_attributes(116-200)src/views/motor.py (1)
MotorSimulation(7-73)
src/services/rocket.py (3)
src/views/motor.py (1)
MotorSimulation(7-73)src/utils.py (1)
collect_attributes(116-200)src/views/rocket.py (1)
RocketSimulation(8-59)
src/utils.py (4)
src/views/environment.py (1)
EnvironmentSimulation(13-78)src/views/flight.py (1)
FlightSimulation(9-140)src/views/motor.py (1)
MotorSimulation(7-73)src/views/rocket.py (1)
RocketSimulation(8-59)
src/views/flight.py (4)
src/views/interface.py (1)
ApiBaseView(4-6)src/views/rocket.py (1)
RocketSimulation(8-59)src/views/environment.py (1)
EnvironmentSimulation(13-78)src/models/flight.py (1)
FlightModel(7-71)
🪛 GitHub Actions: Pylint
src/utils.py
[warning] 57-57: pylint: W0237: Parameter 'o' has been renamed to 'obj' in overriding 'InfinityEncoder.default' method (arguments-renamed)
[warning] 75-75: pylint: W0104: Statement seems to have no effect (pointless-statement)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 116-116: pylint: R0912: Too many branches (23/12) (too-many-branches)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 303-303: pylint: W0101: Unreachable code (unreachable)
[warning] 316-316: pylint: W0101: Unreachable code (unreachable)
[warning] 329-329: pylint: W0101: Unreachable code (unreachable)
🔇 Additional comments (5)
src/models/rocket.py (1)
22-22: Type correction looks good!The change from
inttofloatforcenter_of_mass_without_motoris appropriate as center of mass values are typically floating-point measurements in physical simulations.src/models/sub/aerosurfaces.py (1)
44-45: New sweep parameters for fins look good!The addition of
sweep_lengthandsweep_angleas optional float parameters is appropriate for modeling swept fins in aerodynamic simulations. These will be properly handled by the existingget_additional_parametersmethod.src/services/environment.py (1)
57-61: LGTM! Clean refactor to use the new encoding system.The change to use
collect_attributesaligns well with the overall PR's encoding refactor. The implementation correctly passes theEnvironmentSimulationclass for attribute collection.src/services/motor.py (1)
143-148: LGTM on switching to collect_attributesCentralizes encoding and keeps the public API unchanged. Looks good.
src/services/flight.py (1)
61-71: Risk: FlightSimulation(extra='ignore') may drop aggregated fieldsYou now aggregate many attributes via collect_attributes, but FlightSimulation is configured with extra='ignore'. Any unexpected/new fields will be discarded. Either update FlightSimulation to extra='allow' (preferred) or ensure collect_attributes outputs only declared fields.
| def collect_attributes(obj, attribute_classes=None): | ||
| """ | ||
| Collect attributes from various simulation classes and populate them from the flight object. | ||
| """ | ||
| if attribute_classes is None: | ||
| attribute_classes = [] | ||
|
|
||
| attributes = rocketpy_encoder(obj) | ||
|
|
||
| for attribute_class in attribute_classes: | ||
| if issubclass(attribute_class, FlightSimulation): | ||
| flight_attributes_list = [ | ||
| attr | ||
| for attr in attribute_class.__annotations__.keys() | ||
| if attr not in ["message", "rocket", "env"] | ||
| ] | ||
| try: | ||
| discretized_func = Function(attr_value.source) | ||
| discretized_func.set_discrete( | ||
| lower=config.bounds[0], | ||
| upper=config.bounds[1], | ||
| samples=config.samples, | ||
| mutate_self=True, | ||
| ) | ||
|
|
||
| setattr(obj_copy, attr_name, discretized_func) | ||
|
|
||
| except Exception as e: | ||
| logger.warning(f"Failed to discretize {attr_name}: {e}") | ||
| for key in flight_attributes_list: | ||
| if key not in attributes: | ||
| try: | ||
| value = getattr(obj, key) | ||
| attributes[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass | ||
|
|
||
| elif issubclass(attribute_class, RocketSimulation): | ||
| rocket_attributes_list = [ | ||
| attr | ||
| for attr in attribute_class.__annotations__.keys() | ||
| if attr not in ["message", "motor"] | ||
| ] | ||
| try: | ||
| for key in rocket_attributes_list: | ||
| if key not in attributes.get("rocket", {}): | ||
| try: | ||
| value = getattr(obj.rocket, key) | ||
| attributes.setdefault("rocket", {})[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass | ||
|
|
||
| elif issubclass(attribute_class, MotorSimulation): | ||
| motor_attributes_list = [ | ||
| attr | ||
| for attr in attribute_class.__annotations__.keys() | ||
| if attr not in ["message"] | ||
| ] | ||
| try: | ||
| for key in motor_attributes_list: | ||
| if key not in attributes.get("rocket", {}).get( | ||
| "motor", {} | ||
| ): | ||
| try: | ||
| value = getattr(obj.rocket.motor, key) | ||
| attributes.setdefault("rocket", {}).setdefault( | ||
| "motor", {} | ||
| )[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass | ||
|
|
||
| elif issubclass(attribute_class, EnvironmentSimulation): | ||
| environment_attributes_list = [ | ||
| attr | ||
| for attr in attribute_class.__annotations__.keys() | ||
| if attr not in ["message"] | ||
| ] | ||
| try: | ||
| for key in environment_attributes_list: | ||
| if key not in attributes.get("env", {}): | ||
| try: | ||
| value = getattr(obj.env, key) | ||
| attributes.setdefault("env", {})[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass | ||
| else: | ||
| continue | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and reduce complexity
The collect_attributes function has high cyclomatic complexity (23 branches) and deeply nested blocks. The broad exception catching could hide bugs. Consider:
- Extracting methods for each attribute class type
- Using more specific exception types
- Logging errors instead of silently passing
+import logging
+
+logger = logging.getLogger(__name__)
+
def collect_attributes(obj, attribute_classes=None):
"""
Collect attributes from various simulation classes and populate them from the flight object.
"""
if attribute_classes is None:
attribute_classes = []
attributes = rocketpy_encoder(obj)
for attribute_class in attribute_classes:
if issubclass(attribute_class, FlightSimulation):
- flight_attributes_list = [
- attr
- for attr in attribute_class.__annotations__.keys()
- if attr not in ["message", "rocket", "env"]
- ]
- try:
- for key in flight_attributes_list:
- if key not in attributes:
- try:
- value = getattr(obj, key)
- attributes[key] = value
- except Exception:
- pass
- except Exception:
- pass
+ _collect_flight_attributes(obj, attribute_class, attributes)
elif issubclass(attribute_class, RocketSimulation):
- # ... similar refactor
+ _collect_rocket_attributes(obj, attribute_class, attributes)
elif issubclass(attribute_class, MotorSimulation):
- # ... similar refactor
+ _collect_motor_attributes(obj, attribute_class, attributes)
elif issubclass(attribute_class, EnvironmentSimulation):
- # ... similar refactor
+ _collect_environment_attributes(obj, attribute_class, attributes)
else:
continue
return attributes
+def _collect_flight_attributes(obj, attribute_class, attributes):
+ """Collect flight-specific attributes."""
+ flight_attributes_list = [
+ attr
+ for attr in attribute_class.__annotations__.keys()
+ if attr not in ["message", "rocket", "env"]
+ ]
+ for key in flight_attributes_list:
+ if key not in attributes:
+ try:
+ value = getattr(obj, key)
+ attributes[key] = value
+ except AttributeError:
+ logger.debug(f"Attribute {key} not found on flight object")
+ except Exception as e:
+ logger.warning(f"Error collecting flight attribute {key}: {e}")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
[warning] 116-116: pylint: R0912: Too many branches (23/12) (too-many-branches)
[warning] 125-125: pylint: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
🤖 Prompt for AI Agents
src/utils.py lines 116-199: The collect_attributes function is too complex and
swallows all exceptions; refactor by extracting four small helper functions
(populate_flight_attributes(obj, attributes, attribute_class),
populate_rocket_attributes(...), populate_motor_attributes(...),
populate_environment_attributes(...)) and call them in the main loop with an
early-continue style to avoid deep nesting; replace bare except Exception blocks
with specific catches (AttributeError, TypeError, KeyError) around getattr and
issubclass checks, and surface failures by logging them (add a module logger via
import logging; logger = logging.getLogger(__name__) and use logger.exception or
logger.error) instead of silently passing; ensure helpers use
attributes.setdefault for nested dicts and avoid repeated get(..., {}) checks to
simplify logic.
There was a problem hiding this comment.
that is a very nice refactoring suggestion,
it modularizes the attribute collection distributing complexity out of global collection more evenly between each object
There was a problem hiding this comment.
@GabrielBarberini Thank you! I'm glad the refactoring suggestion resonates with you. Breaking down the complex collect_attributes function into focused helper functions would indeed make the code more maintainable and easier to reason about. The modular approach also makes it easier to test each attribute collection strategy independently and provides better error visibility through specific logging.
src/utils.py
Outdated
| value == b"application/octet-stream" | ||
| for header, value in self.initial_message["headers"] | ||
| ): | ||
| # Don't apply GZip to small outgoing responses or octet-streams. | ||
| await self.send(self.initial_message) | ||
| await self.send(message) # pylint: disable=unreachable | ||
| await self.send(message) |
There was a problem hiding this comment.
Remove unreachable code after returns
The pipeline correctly identified unreachable code after return statements. The await self.send(message) lines at 303, 316, and 329 will never execute.
):
await self.send(self.initial_message)
- await self.send(message)
+ return await self.send(message)
elif not more_body:
self.gzip_file.write(body)
self.gzip_file.close()
body = self.gzip_buffer.getvalue()
headers = MutableHeaders(raw=self.initial_message["headers"])
headers["Content-Encoding"] = "gzip"
headers["Content-Length"] = str(len(body))
headers.add_vary_header("Accept-Encoding")
message["body"] = body
await self.send(self.initial_message)
- await self.send(message)
+ return await self.send(message)
else:
headers = MutableHeaders(raw=self.initial_message["headers"])
headers["Content-Encoding"] = "gzip"
headers.add_vary_header("Accept-Encoding")
del headers["Content-Length"]
self.gzip_file.write(body)
message["body"] = self.gzip_buffer.getvalue()
self.gzip_buffer.seek(0)
self.gzip_buffer.truncate()
await self.send(self.initial_message)
- await self.send(message)
+ return await self.send(message)Also applies to: 303-316, 316-329
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 303-303: pylint: W0101: Unreachable code (unreachable)
🤖 Prompt for AI Agents
In src/utils.py around lines 299-303 (and similarly 303-316 and 316-329), there
are await self.send(message) calls that are unreachable because earlier
returns/exits in the same control flow make those lines dead; remove the
unreachable await self.send(message) calls (or, if the sends are required, move
them so they execute before the returning statements) so no code after a return
remains; ensure tests/logic still send the expected messages by either deleting
the dead lines or reordering to send then return.
| model_config = ConfigDict( | ||
| ser_json_exclude_none=True, extra='allow', arbitrary_types_allowed=True | ||
| ) | ||
| model_config = ConfigDict(extra='ignore', arbitrary_types_allowed=True) |
There was a problem hiding this comment.
extra='ignore' contradicts dynamic schema; enable extra='allow'
Current config will drop unknown keys coming from collect_attributes, undermining the refactor’s goal of accommodating new fields. Align with other views and preserve ser_json_exclude_none.
- model_config = ConfigDict(extra='ignore', arbitrary_types_allowed=True)
+ model_config = ConfigDict(
+ ser_json_exclude_none=True, extra='allow', arbitrary_types_allowed=True
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model_config = ConfigDict(extra='ignore', arbitrary_types_allowed=True) | |
| model_config = ConfigDict( | |
| ser_json_exclude_none=True, extra='allow', arbitrary_types_allowed=True | |
| ) |
🤖 Prompt for AI Agents
In src/views/flight.py around line 19, the model_config currently uses
extra='ignore' which drops unknown keys from collect_attributes; change it to
extra='allow' (keeping arbitrary_types_allowed=True) and also include the
ser_json_exclude_none setting consistent with other views so unknown fields are
preserved and None fields are excluded when serializing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils.py (1)
25-54: Remove the unused DiscretizeConfig class in src/utils.py.
No references outside its definition were found—this dead code can be safely removed.
♻️ Duplicate comments (2)
src/utils.py (2)
303-313: Eliminate “unreachable” lints by returning after sendThese lines are flagged W0101. Return after sending both messages to end the branch cleanly.
await self.send(self.initial_message) - await self.send(message) # pylint: disable=unreachable + await self.send(message) + return @@ await self.send(self.initial_message) - await self.send(message) # pylint: disable=unreachable + await self.send(message) + return @@ await self.send(self.initial_message) - await self.send(message) # pylint: disable=unreachable + await self.send(message) + returnAlso applies to: 319-327, 334-341
77-97: Call post-process, avoid in-place mutation, enforce ≤25, replace SciPy interp
- Missing call:
_Flight__evaluate_post_processis not invoked.- In-place mutation of
Flight.solution/flight_phases/function_evaluationsinside an encoder risks state corruption for callers.- Same floor-div bug as above; use a fixed
target_samples.- Replace
interp1dwithnp.interpto drop SciPy.- if isinstance(obj, Flight): - obj._Flight__evaluate_post_process - solution = np.array(obj.solution) - size = len(solution) - if size > 25: - reduction_factor = size // 25 - reduced_solution = np.zeros( - (size // reduction_factor, solution.shape[1]) - ) - reduced_scale = np.linspace( - solution[0, 0], solution[-1, 0], size // reduction_factor - ) - for i, col in enumerate(solution.T): - reduced_solution[:, i] = interp1d( - solution[:, 0], col, assume_sorted=True - )(reduced_scale) - obj.solution = reduced_solution.tolist() - - obj.flight_phases = None - obj.function_evaluations = None + if isinstance(obj, Flight): + obj._Flight__evaluate_post_process() + solution = np.array(obj.solution) + size = len(solution) + target_samples = 25 if size > 25 else size + # snapshot originals to avoid leaking mutations + orig_solution = obj.solution + orig_fp = getattr(obj, "flight_phases", None) + orig_fe = getattr(obj, "function_evaluations", None) + try: + if target_samples and target_samples != size: + reduced_solution = np.zeros((target_samples, solution.shape[1])) + reduced_scale = np.linspace(solution[0, 0], solution[-1, 0], target_samples) + for i, col in enumerate(solution.T): + reduced_solution[:, i] = np.interp(reduced_scale, solution[:, 0], col) + obj.solution = reduced_solution.tolist() + # Always mask heavy internals + obj.flight_phases = None + obj.function_evaluations = None + return super().default(obj) + finally: + obj.solution = orig_solution + obj.flight_phases = orig_fp + obj.function_evaluations = orig_fe
🧹 Nitpick comments (6)
src/utils.py (6)
11-13: Prefer public import path for RocketPyEncoderImporting from a private module (
rocketpy._encoders) is brittle across versions. If available, switch to a public path.Would this work in your environment?
-from rocketpy._encoders import RocketPyEncoder +from rocketpy.encoders import RocketPyEncoder # or: from rocketpy import RocketPyEncoder
8-10: Drop SciPy dependency; use numpy.interpThis path only needs 1D interpolation;
np.interpis sufficient and avoids a heavy dependency/import on request paths.Apply:
-import numpy as np -from scipy.interpolate import interp1d +import numpy as np(see Flight reduction change below for replacing
interp1d.)
56-59: Unnecessary init override
InfinityEncoder.__init__just calls super; you can drop it.-class InfinityEncoder(RocketPyEncoder): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) +class InfinityEncoder(RocketPyEncoder): ...
60-61: Match base signature to silence Pylint W0237Rename parameter to match the base encoder (
default(self, o)).- def default(self, obj): + def default(self, o): + obj = o
119-204: Simplify collect_attributes; narrow exception scopesCurrent broad try/except blocks hide real bugs and trigger pylint complexity warnings. Narrow to
AttributeErrorand drop outer no-op catches where possible.- try: - for key in flight_attributes_list: - if key not in attributes: - try: - value = getattr(obj, key) - attributes[key] = value - except Exception: - pass - except Exception: - pass + for key in flight_attributes_list: + if key not in attributes: + try: + attributes[key] = getattr(obj, key) + except AttributeError: + pass @@ - try: - for key in rocket_attributes_list: - if key not in attributes.get("rocket", {}): - try: - value = getattr(obj.rocket, key) - attributes.setdefault("rocket", {})[key] = value - except Exception: - pass - except Exception: - pass + for key in rocket_attributes_list: + if key not in attributes.get("rocket", {}): + try: + value = getattr(obj.rocket, key) + attributes.setdefault("rocket", {})[key] = value + except AttributeError: + pass @@ - try: - for key in motor_attributes_list: - if key not in attributes.get("rocket", {}).get( - "motor", {} - ): - try: - value = getattr(obj.rocket.motor, key) - attributes.setdefault("rocket", {}).setdefault( - "motor", {} - )[key] = value - except Exception: - pass - except Exception: - pass + for key in motor_attributes_list: + if key not in attributes.get("rocket", {}).get("motor", {}): + try: + value = getattr(obj.rocket.motor, key) + attributes.setdefault("rocket", {}).setdefault("motor", {})[key] = value + except AttributeError: + pass @@ - try: - for key in environment_attributes_list: - if key not in attributes.get("env", {}): - try: - value = getattr(obj.env, key) - attributes.setdefault("env", {})[key] = value - except Exception: - pass - except Exception: - pass + for key in environment_attributes_list: + if key not in attributes.get("env", {}): + try: + value = getattr(obj.env, key) + attributes.setdefault("env", {})[key] = value + except AttributeError: + passAlso verify that returning
rocketpy_encoder(attributes)won’t recurse (looks safe now).
101-116: Add backward-compatibility and verifyallow_picklesupport
- Change
rocketpy_encodersignature to accept aconfig=Noneparameter (ignored) to avoid breaking callers.- Confirm that
allow_pickleis accepted byInfinityEncoder/RocketPyEncoder(nativejson.dumpsdoesn’t support it by default).-def rocketpy_encoder(obj): +def rocketpy_encoder(obj, config=None): """ Encode a RocketPy object using official RocketPy encoders. + `config` is accepted for backward compatibility and ignored. """ json_str = json.dumps( obj, cls=InfinityEncoder, include_outputs=True, include_function_data=True, discretize=True, - allow_pickle=False, + allow_pickle=False, # ensure support in InfinityEncoder/RocketPyEncoder )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.gitignore(1 hunks)src/utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils.py (8)
src/services/environment.py (3)
environment(42-43)environment(46-47)get_environment_simulation(49-61)src/views/environment.py (1)
EnvironmentSimulation(13-78)src/services/flight.py (3)
flight(47-48)flight(51-52)get_flight_simulation(51-62)src/views/flight.py (2)
FlightSimulation(9-140)FlightSimulation(9-107)src/services/motor.py (3)
motor(129-130)motor(133-134)get_motor_simulation(136-145)src/views/motor.py (1)
MotorSimulation(7-73)src/services/rocket.py (3)
rocket(97-98)rocket(101-102)get_rocket_simulation(103-114)src/views/rocket.py (1)
RocketSimulation(8-59)
🪛 GitHub Actions: Pylint
src/utils.py
[warning] 60-60: Pylint: W0237 - Parameter 'o' has been renamed to 'obj' in overriding 'InfinityEncoder.default' method (arguments-renamed).
[warning] 78-78: Pylint: W0104 - Statement seems to have no effect (pointless-statement).
[warning] 119-119: Pylint: R0912 - Too many branches (23/12) (too-many-branches).
[warning] 128-128: Pylint: R1702 - Too many nested blocks (6/5) (too-many-nested-blocks).
[warning] 128-128: Pylint: R1702 - Too many nested blocks (6/5) (too-many-nested-blocks).
[warning] 128-128: Pylint: R1702 - Too many nested blocks (6/5) (too-many-nested-blocks).
🔇 Additional comments (1)
src/utils.py (1)
17-21: LGTM: updated view imports align with attribute collectionThe imports for EnvironmentSimulation, FlightSimulation, MotorSimulation, and RocketSimulation look consistent with the new collection flow.
| if ( | ||
| isinstance(obj, Function) | ||
| and not callable(obj.source) | ||
| and obj.__dom_dim__ == 1 | ||
| ): | ||
| size = len(obj._domain) | ||
| reduction_factor = 1 | ||
| if size > 25: | ||
| reduction_factor = size // 25 | ||
| if reduction_factor > 1: | ||
| obj = obj.set_discrete( | ||
| obj.x_array[0], | ||
| obj.x_array[-1], | ||
| size // reduction_factor, | ||
| mutate_self=False, | ||
| ) | ||
| if isinstance(obj, Flight): |
There was a problem hiding this comment.
Function downsampling doesn’t enforce ≤25 for sizes 26–49; avoid private attr
size // 25 floors to 1 when 26 ≤ size ≤ 49, so no reduction happens. Also prefer public arrays over _domain when available.
- if (
+ if (
isinstance(obj, Function)
and not callable(obj.source)
and obj.__dom_dim__ == 1
):
- size = len(obj._domain)
- reduction_factor = 1
- if size > 25:
- reduction_factor = size // 25
- if reduction_factor > 1:
- obj = obj.set_discrete(
- obj.x_array[0],
- obj.x_array[-1],
- size // reduction_factor,
- mutate_self=False,
- )
+ size = len(getattr(obj, "x_array", [])) or len(getattr(obj, "_domain", []))
+ target_samples = 25 if size > 25 else size
+ if target_samples and target_samples != size:
+ obj = obj.set_discrete(
+ obj.x_array[0],
+ obj.x_array[-1],
+ target_samples,
+ mutate_self=False,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| isinstance(obj, Function) | |
| and not callable(obj.source) | |
| and obj.__dom_dim__ == 1 | |
| ): | |
| size = len(obj._domain) | |
| reduction_factor = 1 | |
| if size > 25: | |
| reduction_factor = size // 25 | |
| if reduction_factor > 1: | |
| obj = obj.set_discrete( | |
| obj.x_array[0], | |
| obj.x_array[-1], | |
| size // reduction_factor, | |
| mutate_self=False, | |
| ) | |
| if isinstance(obj, Flight): | |
| if ( | |
| isinstance(obj, Function) | |
| and not callable(obj.source) | |
| and obj.__dom_dim__ == 1 | |
| ): | |
| size = len(getattr(obj, "x_array", [])) or len(getattr(obj, "_domain", [])) | |
| target_samples = 25 if size > 25 else size | |
| if target_samples and target_samples != size: | |
| obj = obj.set_discrete( | |
| obj.x_array[0], | |
| obj.x_array[-1], | |
| target_samples, | |
| mutate_self=False, | |
| ) | |
| if isinstance(obj, Flight): |
🤖 Prompt for AI Agents
In src/utils.py around lines 61–77, the downsampling logic uses size =
len(obj._domain) and reduction_factor = size // 25 which floors to 1 for sizes
26–49 and also accesses a private _domain; change to compute size from a public
attribute when available (e.g. getattr(obj, "domain", getattr(obj, "x_array",
obj._domain))) and compute reduction_factor = math.ceil(size / 25) (import math
if needed), then apply downsampling only when reduction_factor > 1 using the
computed reduction_factor (keeping the existing set_discrete call but using the
new size calculation) so sizes 26–49 are correctly reduced and private
attributes are avoided when possible.
|
|
||
| message: str = "Flight successfully simulated" | ||
|
|
||
| # Core Flight attributes (always present) |
There was a problem hiding this comment.
duplicated comment?
| ): | ||
| size = len(obj._domain) | ||
| reduction_factor = 1 | ||
| if size > 25: |
There was a problem hiding this comment.
maybe this can be simplified to single line with ternary approach
| attributes.setdefault("rocket", {})[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: |
There was a problem hiding this comment.
I don't think there is space for any exception in the search, there is explicit default to {} already if 'rocket' is not found,
so I think we should be able to remove this extra try catch
also, I would prefer to be more specific if possible to which exception we are comfortable skipping and which we need to raise, maybe not all of them should be skipped (at the expense of possibly compromising simulation integrity)
| )[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: |
| attributes.setdefault("env", {})[key] = value | ||
| except Exception: | ||
| pass | ||
| except Exception: |
| return _fix_datetime_fields(encoded_result) | ||
|
|
||
|
|
||
| def collect_attributes(obj, attribute_classes=None): |
There was a problem hiding this comment.
would you mind to set the signature as Union[FlightSim, EnvSim, ...) ?
There was a problem hiding this comment.
Thank you for the enhancement @aasitvora99 !
Globally I think we finally have the opportunity to take major advantage of the fact we use latest python version with pydantic and use some of its fancy pattern matching capabilities instead of the if elifs, see a quick example here: https://youtu.be/MfUuHYi2jgY?t=1204
What would you say to that idea?
Aside from that I left just some minor comments
Fixes
/simulateroute long response time by limiting the samples to25Temprary workaround for jarvis's alpha release.
Summary by CodeRabbit
New Features
Changes
Refactor
Chores