Skip to content
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

ENH: Allow for time variable density to Tank Fluids. #593

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

phmbressan
Copy link
Collaborator

@phmbressan phmbressan commented May 3, 2024

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Currently, the Tank class assumes in its computations that its fluid densities are a constant in time. This approximation can be rather inaccurate, specially for tanks that experiment large variations in fluid density (e.g. due to changes in temperature and pressure) during the flow.

New behavior

This PR essentially allows for time variable densities when defining the Fluid and the Tank subclasses discretize these values in time for code speed.

Breaking change

  • Yes
  • No

Additional Information

The tests for the Tank class would benefit a lot from a refactor. However, I preferred to keep this PR focused on the new functionalities.

@phmbressan phmbressan added the Enhancement New feature or request, including adjustments in current codes label May 3, 2024
@phmbressan phmbressan added this to the Release v1.X.0 milestone May 3, 2024
@phmbressan phmbressan self-assigned this May 3, 2024
@phmbressan phmbressan requested a review from a team as a code owner May 3, 2024 13:22
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.03%. Comparing base (43b2b0d) to head (e020aaa).
Report is 1 commits behind head on develop.

Files Patch % Lines
rocketpy/motors/fluid.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #593   +/-   ##
========================================
  Coverage    73.03%   73.03%           
========================================
  Files           57       57           
  Lines         9596     9609   +13     
========================================
+ Hits          7008     7018   +10     
- Misses        2588     2591    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +19 to +24
density : int, float, callable, string, array, Function
Density of the fluid in kg/m³. If a int or float is given,
it is considered a constant in time. A callable, csv file
or an list of points can be given to express the density
as a function of time. The parameter is used as a ``Function``
source.
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR May 5, 2024

Choose a reason for hiding this comment

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

I believe it makes no sense to have a fluid density depending on the time. Please correct me if I'm wrong, but how can someone predict this function in the first place?

Wouldn't be more appropiate to have this density parameter depending on the temperature and pressure? To create a function like this:

def density(T, P):
    # so some interpolation
    return value

This could be created with the assistance of 2D Function objects and .csv files.

We could even include some "common" fluids csv files in the package, so the user could just call the function with the fluid name and the function would load the csv file and return the 2D Function object.

In case the user still wants to use a constant value, it would still be possible.

Copy link
Collaborator Author

@phmbressan phmbressan May 5, 2024

Choose a reason for hiding this comment

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

Thanks for your suggestion, I considered it at first, but I was hesitant about it for the following reasons. The Tanks module currently does not calculate any thermodynamical quantities.

Correct me if I am mistaken, but unless the Temperature and Pressure in your comment depend on time, we cannot compute anything with it.

At this point, if the Temperature and Pressure are know time variable quantities, the density is also known as a time variable quantity.

Furthermore, the Pressure and Temperature are not used anywhere is Tanks simulation, we would be introducing two new quantities that are not necessary.

I can see two ways of getting the Fluid density in a static fire:

  • Having sensors that compute two thermodynamical properties (such as temperature and pressure) and inferring the others from thermodynamical models;
  • Having sensors for mass flow and fluid level inside the tank, then the density can be computed.

In my view, in both ways it rather easy to get the density as a function of time. If we implement it from pressure and temperature, we limit ourserves to a particular case (but a really common one) of the first option.

I understand what you mean by having the common Fluids in a file and we making the conversion, but I believe this step into thermodynamical territory and this is a bit against our initial intention for the Tank class. Of course, there is always a place to start improving and adding new features/ideas.

Let me know your opinion on the matter.

Copy link
Member

Choose a reason for hiding this comment

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

Can we make Fluid objects motor-agnostic? If fluid's properties depend on time, this time might be associated with a particular motor, so the same fluid couldn't be used in other tanks.

I was thinking about this:

  • You have the volume of the tank, this is always a fixed value.
  • By each time, you calculate the mass of fluid multiplying the volume by the density,
  • This density would actually depend on pressure and temperature.

I know that different types of liquid motors will be defined based on different types of input (liquid level, pressure, mass, etc.), but that would be something to be carefully worked on.

What really complicates the whole thing is that we have both liquids and gases defined as a Fluid, but those may present quite different behaviors.

Comment on lines +30 to 31
density: Union[int, float, str, list, Function]

Copy link
Member

Choose a reason for hiding this comment

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

Python functions could also be accepted, right? (for example the lambda you applied in the example)

I would let callable be a valid type for the density parameter instead of just rocketpy.Function

Suggested change
density: Union[int, float, str, list, Function]
density: Union[int, float, str, list, callable]

Comment on lines +491 to +492
def _discretize_fluids(self):
"""Discretizes the fluid densities according to the flux time and the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _discretize_fluids(self):
"""Discretizes the fluid densities according to the flux time and the
def _discretize_fluidd_density(self):
"""Discretizes the fluid densities according to the flux time and the

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much better, thanks for the suggestion, I will implement it.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Thank you for proposing these changes.
I think everyone is uncomfortable with the current situation regarding constant density.

Since we are proposing a variable fluid density, how about plotting the density evolution somewhere?

@phmbressan phmbressan marked this pull request as draft May 15, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants