MNT: add error handling for static margin calculation #776
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #776 +/- ##
===========================================
+ Coverage 76.42% 80.10% +3.68%
===========================================
Files 95 96 +1
Lines 11090 11368 +278
===========================================
+ Hits 8475 9106 +631
+ Misses 2615 2262 -353 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try: | ||
| self.static_margin.set_discrete( | ||
| lower=0, upper=self.motor.burn_out_time, samples=200 | ||
| ) |
There was a problem hiding this comment.
I agree with the PR since it solves a real and important issue. However, there are a few things I believe it might be worthwhile pointing out:
- How do we determine whether a method/attribute is important enough to have an explicit exception message? Of course, in this PR the
static_margininvolves loads of calculations using other parameters, but I find it hard to pinpoint when it should have the exception handling. - I like that this exception message points to some potential attributes that are sources of the issue. However, the core of the message (i.e.
f"Error in parameter {x}, check inputs.") is almost so generic that it could be applied everywhere in RocketPy. Should we even raise these kinds of exceptions?
There was a problem hiding this comment.
I believe you're "overcomplicating" the situation a little bit.
I like to think this way: Regression! If someone faced an issue while running a simulation, in order to avoid future users to face the same problem, I'm adding a more delicate message suggesting what is probably wrong in the code.
The reason we don't do that for every input? Simple: because no one has complained about other inputs yet.
Try to read #773, you're going to see that the source of the error is quite silent, so a message like this can guide the user to quickly find where to look in the code.
If the input is clearly wrong, for example, someone sets a negative mass to the rocket, I think that would be the case we wouldn't require much effort from our team (or the user) to identify the error source. However the case of this PR is particularly trickly given that everything was properly set in the simulation, but rocketpy only crashed because we couldn't perform a single operation.
Additionally, the try/catch block barely changes the execution time, so I see almost no problem of doing so.
Lemme know ehat you think about it.
|
In the end, what exactly was causing the computation to break? Simulating with burn time longer then the provided curves should not break things in theory right? We really have to turn the static margin calculation into something lazy. There is no need to have this calculated for the simulation, it should only be computed when needed |
@MateusStano the way the user have set the hybrid motor made the center of mass function undefined after ~14s. |
|
@phmbressan @MateusStano I'm re-requesting the review on this PR. I believe we are "almost done" here |
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior
See #773
New behavior
Simple message added, I hope this helps future users
Breaking change
Additional information
Enter text here...