-
-
Notifications
You must be signed in to change notification settings - Fork 1
ENH: class encoding refactor for large sims #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9339a43
fb23890
ba7c28b
7c7f805
6ecc358
5eab472
8450082
c295651
42afc99
4368487
26f510d
52e09ca
a15232d
a8862c4
bafd992
add32b9
732f167
fab9582
11110c0
f4ddde1
de30435
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,16 +2,23 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import io | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import copy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from datetime import datetime | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import NoReturn, Tuple | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rocketpy import Function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from scipy.interpolate import interp1d | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rocketpy import Function, Flight | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rocketpy._encoders import RocketPyEncoder | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from starlette.datastructures import Headers, MutableHeaders | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from starlette.types import ASGIApp, Message, Receive, Scope, Send | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from src.views.environment import EnvironmentSimulation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from src.views.flight import FlightSimulation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from src.views.motor import MotorSimulation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from src.views.rocket import RocketSimulation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -46,78 +53,154 @@ def for_flight(cls) -> 'DiscretizeConfig': | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return cls(bounds=(0, 30), samples=200) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def rocketpy_encoder(obj, config: DiscretizeConfig = DiscretizeConfig()): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Encode a RocketPy object using official RocketPy encoders. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class InfinityEncoder(RocketPyEncoder): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This function creates a copy of the object, discretizes callable Function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attributes on the copy, and then uses RocketPy's official RocketPyEncoder for | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| complete object serialization. The original object remains unchanged. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 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): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+61
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function downsampling doesn’t enforce ≤25 for sizes 26–49; avoid private attr
- 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj._Flight__evaluate_post_process | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj._Flight__evaluate_post_process | |
| obj._Flight__evaluate_post_process() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be a legit statement from copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind to set the signature as Union[FlightSim, EnvSim, ...) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Copilot
AI
Aug 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this can be simplified to single line with ternary approach