ENH: Spherical Caps Included in Total Length

[MateusStano]

Pull request type

• Code changes (bugfix, features)
• Code maintenance (refactoring, formatting, tests)
• ReadMe, Docs and GitHub updates
• Other (please describe):

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 --runslow) have passed locally

Current behavior

When creating a cylindrical tank with spherical caps, the height argument only refers to the height of the tank, excluding the tank. This is counterintuitive and just forces the user to measure/calculate impractical stuff.

Also, this was not explicit in the documentation

New behavior

The height argument now refers to the entire tank, with or without spherical caps

Breaking change

• Yes
• No

Additional information

Seems there was also a bug with some geometries due to numerical errors in the calculations of the spherical caps, so really long tanks could not be defined. This was fixed by adding an abs to the calculation of the spherical caps geometry

[Gui-FernandesBR]

I understand your motivation and it is fair enough to me.

However, this PR technically introduces a breaking change since the add_spherical_caps is not a private method and you're adding a new mandatory argument to it. I recommend removing the new total_height argument. but it would also be possible to set this argument as optional.

Also, the code is considering that the caps will have a perfect sphere at the its both tips. If you give a blind search, it's almost impossible to find a perfect sphere format (https://www.google.com/search?q=cylindrical+tank&sca_esv=581032301&tbm=isch&source=lnms&sa=X&ved=2ahUKEwiw-9vHl7iCAxUpJrkGHYFKBBIQ_AUoAXoECAIQAw&biw=1358&bih=642&dpr=1). Usually it has a height different than the tank radius. Shall we start considering this phenome as well?

[MateusStano]

@Gui-FernandesBR @phmbressan added the warning as we talked

Please give a re-review here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@c7c3e2a). Click here to learn what that means.

Additional details and impacted files

@@            Coverage Diff             @@
##             develop     #455   +/-   ##
==========================================
  Coverage           ?   69.51%           
==========================================
  Files              ?       55           
  Lines              ?     8999           
  Branches           ?        0           
==========================================
  Hits               ?     6256           
  Misses             ?     2743           
  Partials           ?        0           

Flag	Coverage Δ
unittests	69.51% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[phmbressan]

Good, this way the object creation seems much more intuitive. The docstrings are also much clearer.

[Gui-FernandesBR]

Nice problem solving, the code is clearer now, I'm comfortable with its final state.