Skip to content

HOTFIX: Tanks Overfill not Being Detected #479

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.

Sign up for GitHub

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

Merged
MateusStano merged 6 commits into from
Nov 23, 2023
Merged

HOTFIX: Tanks Overfill not Being Detected #479

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
88a2991
BUG: fix Function compose extrapolation check.
phmbressan Nov 21, 2023
09260e4
FIX: MassBasedTank out of bounds check not working.
phmbressan Nov 21, 2023
9f69c74
TST: fix test of MassBasedTank overfill.
phmbressan Nov 21, 2023
d1a5dd5
DOC: update SEB doc with correct density values.
phmbressan Nov 21, 2023
21e9f58
STY: apply black for linting.
phmbressan Nov 21, 2023
6894dd4
DOC: improve MassBasedTank over volume error message.
phmbressan Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rocketpy/mathutils/function.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2715,7 +2715,7 @@ def compose(self, func, extrapolate=False):
if isinstance(self.source, np.ndarray) and isinstance(func.source, np.ndarray):
# Perform bounds check for composition
if not extrapolate:
if func.min < self.x_initial and func.max > self.x_final:
if func.min < self.x_initial or func.max > self.x_final:
raise ValueError(
f"Input Function image {func.min, func.max} must be within "
f"the domain of the Function {self.x_initial, self.x_final}."
Expand Down
15 changes: 14 additions & 1 deletion rocketpy/motors/tank.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,20 @@ def fluid_volume(self):
Function
Volume of the fluid as a function of time.
"""
return self.liquid_volume + self.gas_volume
fluid_volume = self.liquid_volume + self.gas_volume

# Check if within bounds
diff = fluid_volume - self.geometry.total_volume

if (diff > 1e-6).any():
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."

@giovaniceotto giovaniceotto Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@phmbressan phmbressan Nov 22, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@phmbressan phmbressan Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@giovaniceotto giovaniceotto Nov 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phmbressan with np.argmin, don't you have the risk of getting a negative value, in which case the tank is underfilled?

@phmbressan phmbressan Nov 23, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check here is made following these assumptions:

  1. The diff pointed out that the Tank is, at some time, overfilled;
  2. There is a value of time that the fluid volume surpassed the total volume of the Tank;
  3. 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.

@MateusStano MateusStano Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

return fluid_volume

@funcify_method("Time (s)", "Volume (m³)")
def gas_volume(self):
Expand Down