HOTFIX: Tanks Overfill not Being Detected #479
Conversation
phmbressan
added
Bug
| (0.0559, 0.7139): lambda y: 0.0744, | ||
| (0.7139, 0.7698): top_endcap, | ||
| (0.0559, 0.8039): lambda y: 0.0744, | ||
| (0.8039, 0.8698): top_endcap, |
There was a problem hiding this comment.
Comment on these changes: after making the assertion from lines 192-195, the tests failed due to the fact that the defined geometry volume is too low. Therefore the tank ended up being overfilled and had to be increased in size.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## hotfix/v1.1.1 #479 +/- ##
================================================
Coverage ? 70.83%
================================================
Files ? 55
Lines ? 9240
Branches ? 0
================================================
Hits ? 6545
Misses ? 2695
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rocketpy/motors/tank.py
Outdated
| "The `fluid_volume`, defined as the sum of `gas_volume` and " | ||
| + "`liquid_volume`, is not equal to the total volume of the tank." | ||
| + "\n\t\tThe difference is more than 1e-6 m^3 at " | ||
| + f"{diff.x_array[np.argmin(diff.y_array)]} s." |
There was a problem hiding this comment.
Just checking: we need to guarantee, for now, that fluid_volume does not exceed self.geometry.total_volume, but we don't really care yet if fluid_volume is much less then self.geometry.total_volume?
If this is the case, shouldn't we use np.argmax instead of np.argmin?
There was a problem hiding this comment.
I do think you are right regarding np.argmax, but, just to make sure we are on the same page, this would need to be changed in all error messages of this class, right?
There was a problem hiding this comment.
Actually, I correct myself: the computation should be np.argmin since we are looking for the smallest of the differences between the fluid volume and the tank volume to determine exactly where it overfilled.
There was a problem hiding this comment.
@phmbressan with np.argmin, don't you have the risk of getting a negative value, in which case the tank is underfilled?
There was a problem hiding this comment.
The check here is made following these assumptions:
- The
diffpointed out that theTankis, at some time, overfilled; - There is a value of time that the fluid volume surpassed the total volume of the Tank;
- In general, this specific point in time is not part of the time samples so the minimum value of the diff is used to select it. I do think it is possible that this value may be a bit before the overfill crossing or a bit after, so there is a certain margin of error.
Perhaps I should mention that the time is approximate or, likely a better solution, use Function.find_input. I favored the implemented way due the fact this is a hotfix and the np.argmin was already being used in the other exceptions of this class.
Let me know if I made it clearer now or if there is any mistake in my reasoning.
rocketpy/motors/tank.py
Outdated
| raise ValueError( | ||
| "The `fluid_volume`, defined as the sum of `gas_volume` and " | ||
| + "`liquid_volume`, is not equal to the total volume of the tank." | ||
| + "\n\t\tThe difference is more than 1e-6 m^3 at " | ||
| + f"{diff.x_array[np.argmin(diff.y_array)]} s." |
There was a problem hiding this comment.
I think this error message could be improved a bit
This is a MassBasedTank, meaning the user will have given mass curves to the tank. If this error pops up warning about volume issues, the user might get confused and the correction to this error will not be clear.
I would suggest telling the user that the masses, the densities or the tank geometry might be wrong
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest --runslow) have passed locallyCurrent behavior
The
MassBasedTankdid not compute correctly overfills or underfills due to some code checks that were not working properly, including:Function.composeextrapolation check was not being applied correctly;gas_height, however it is more accurate to check before computations influid_volume.New behavior
The main changes this hotfix introduces:
Function.composeextrapolation was corrected;MassBasedTank.fluid_volume;test_mass_based_tankregarding thefluid_volumeso as to prevent this kind of error in the near future;Breaking change