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

MNT: Refactor Tank's testing Assertion with CAD data. #678

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

Conversation

phmbressan
Copy link
Collaborator

@phmbressan phmbressan commented Aug 30, 2024

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • 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's testing module present the following issues:

  • The module has too complex logic inside the tests;
  • There are analytical functions that perform geometrical validation:
    • Those, even if correct, are hard to maintain and may, for instance, repeat mistakes from the code.

New behavior

This PR aims to bring the following changes to code:

  • CAD related data by cadquery module for geometrical validation;
  • Refactor of the complex logic inside the Tanks tests;
    • There is still room for improvement here, but the code is better organized now.
  • Implement plotting and printing functionality to the Tanks class (this can be moved to another PR if needed).

Breaking change

  • Yes
  • No

Additional information

Check out CadQuery docs for further information and examples.

An important notice is that the CAD module is not executed in every test for the two reasons:

  • The CAD library has a large install size (> 200MB);
  • Some models are slow to be used in parametrized tests.

Therefore I have added the necessary code into a Jupyter notebook for easy recalculation of the expected values, but this can be discussed further on this PR.

P.S.: I will fix the PyLint errors shortly.

@phmbressan phmbressan added this to the Release v1.X.0 milestone Aug 30, 2024
@phmbressan phmbressan self-assigned this Aug 30, 2024
@phmbressan phmbressan requested a review from a team as a code owner August 30, 2024 21:14
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 96.73913% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.35%. Comparing base (3a4c742) to head (8c6eefa).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/plots/tank_geometry_plots.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #678      +/-   ##
===========================================
+ Coverage    75.87%   76.35%   +0.48%     
===========================================
  Files           85       85              
  Lines        10085    10131      +46     
===========================================
+ Hits          7652     7736      +84     
+ Misses        2433     2395      -38     

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

self.tank.net_mass_flow_rate.plot(*self.flux_time)
self.fluid_height()
self.fluid_volume()
self.fluid_center_of_mass()
Copy link
Member

Choose a reason for hiding this comment

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

You should add an .all_info and .info method to the Tanks class and Fluids class now that the plots and prints are good

Copy link
Member

Choose a reason for hiding this comment

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

I actually quite like this solution, seems to avoid some annoyances while being very pratical. I would suggest that you rename this file to something like tank_cad_validation_data_generator.ipynb or something of the sort for clarity purposes.

Or maybe just add a markdown cell with one or two sentences explaining what the notebook is for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great comment. I will do that.

The notebook is still a bit raw in terms of explanations, since I opted to wait for opinions on whether we'd keep it.

@Gui-FernandesBR Gui-FernandesBR changed the title MNT: Refactor Tank's testing Assertion with CAD data. TST: Refactor Tank's testing Assertion with CAD data. Sep 8, 2024
@Gui-FernandesBR Gui-FernandesBR changed the title TST: Refactor Tank's testing Assertion with CAD data. MNT: Refactor Tank's testing Assertion with CAD data. Sep 8, 2024
@Gui-FernandesBR Gui-FernandesBR marked this pull request as draft September 12, 2024 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants