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: Environment class major refactor may 2024 #605

Merged
merged 82 commits into from
Jul 12, 2024

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented May 20, 2024

Pull request type

  • Code maintenance (refactoring, formatting, tests)

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

Reading and maintaining the Environment class is not an easy task.

New behavior

First of all, I'm reeeeeeeeally sorry for the size of this PR. These are the overall highlights of my changes:

Enhancements (ENH)

  • Created rocketpy.environment.tools module.
  • Created the environment.fetchers module.
  • Created a WeatherModelMapping class.
  • Added modulo operator to Function class.
  • Added auxiliary private setters to the Environment class.
  • Added a few input validation functions to the Environment class.

Documentation (DOC)

  • Reviwed the entire class docstrings!! Many errors were fixed.

Bug Fixes (BUG)

  • The CMC Ensemble model is no longer available, so I removed it and updated the tests. In the future I can re-implement it with the new API endpoint.

Maintenance (MNT)

  • Refactored almost 99% of the Environment class.
  • Fixed pylint and flake8 errors in the code.
  • Code is cleaner, lighter and more readable! The environment.py file now has less than 3l code lines.

Testing (TST)

  • Updated all the Environment tests.
  • Re-evaluated slow tests and optimized them.
  • I reduced the execution time and increased the coverage of the tests.

Breaking change

  • No (at least not hard breaking changes).

Additional information

Now it's official... We have remodeled all the original RocketPy classes.

Review:

To better review this PR I recommend:

  • Run the slow tests locally (if it fails, please comment it here so we can reproduce it.)
  • Run the getting_started notebook
  • Run the Environment class docstrings and usage notebook.
  • Read the codecoverage report and search for potential gaps that are not being covered by the tests
  • You can ignore all the modifications in the tests module, since it is easy to fix it later.
  • Remember that we have pylint and flake8 already passing in the CI
  • Try to playaround... Import the environment class and create different objects simulating the user behavior.
  • If you have to prioritize which files to read, I recommend you to read the new files that are being added.

Gui-FernandesBR and others added 30 commits May 17, 2024 01:43
Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 60.41667% with 133 lines in your changes missing coverage. Please review.

Project coverage is 75.68%. Comparing base (44cd770) to head (1c6f15b).
Report is 8 commits behind head on develop.

Files Patch % Lines
rocketpy/environment/fetchers.py 27.97% 103 Missing ⚠️
rocketpy/environment/tools.py 85.02% 25 Missing ⚠️
rocketpy/mathutils/function.py 50.00% 3 Missing ⚠️
rocketpy/environment/weather_model_mapping.py 87.50% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #605      +/-   ##
===========================================
+ Coverage    73.85%   75.68%   +1.83%     
===========================================
  Files           70       72       +2     
  Lines        10035     9773     -262     
===========================================
- Hits          7411     7397      -14     
+ Misses        2624     2376     -248     

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

@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review July 6, 2024 21:36
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner July 6, 2024 21:36
rocketpy/environment/__init__.py Show resolved Hide resolved
rocketpy/environment/tools.py Outdated Show resolved Hide resolved
rocketpy/environment/tools.py Outdated Show resolved Hide resolved
rocketpy/environment/tools.py Outdated Show resolved Hide resolved
rocketpy/environment/tools.py Outdated Show resolved Hide resolved
rocketpy/environment/tools.py Outdated Show resolved Hide resolved
rocketpy/environment/tools.py Show resolved Hide resolved
rocketpy/environment/environment.py Show resolved Hide resolved
rocketpy/environment/environment.py Outdated Show resolved Hide resolved
rocketpy/environment/environment.py Show resolved Hide resolved
rocketpy/environment/fetchers.py Outdated Show resolved Hide resolved
rocketpy/environment/fetchers.py Outdated Show resolved Hide resolved
rocketpy/environment/fetchers.py Show resolved Hide resolved
@Lucas-Prates
Copy link
Contributor

Lucas-Prates commented Jul 9, 2024

I have given only a very small partial review, I will complete it in time. Here are some points other than the few comments made in code:

  1. There was a single assertion error wen running pytest-slow, see below;
    image

  2. The getting started notebook ran smoothly;

  3. The environment analysis usage notebook has a CMC Ensemble section that does not run. From what I understood from the documentation of set_atmospheric_model, since CMC Ensembles are currently unavailable, should this section be removed?

  4. I mostly "played around" with stuff, so I haven't read thoroughly the code yet.

@Gui-FernandesBR
Copy link
Member Author

I have given only a very small partial review, I will complete it in time. Here are some points other than the few comments made in code:

  1. There was a single assertion error wen running pytest-slow, see below;
    image

I can reproduce this error in my machine.
This is odd and it is not related to this PR. I guess it was introduced in the #628, given the commit history.
I think I will fix the test in this PR and we can move on.
We didn`t change anything in the EnvironmentAnalysis class, so it is for sure a problem with the test itself.

  1. The getting started notebook ran smoothly;
  2. The environment analysis usage notebook has a CMC Ensemble section that does not run. From what I understood from the documentation of set_atmospheric_model, since CMC Ensembles are currently unavailable, should this section be removed?

I will make another PR to update environment documentation. Work in progress.

  1. I mostly "played around" with stuff, so I haven't read thoroughly the code yet.

Thanks for your review!! Your points were really helpful.

@Gui-FernandesBR
Copy link
Member Author

@MateusStano @Lucas-Prates I have addressed all of your comments.

Please revisit the resolved conversations and approve the PR if you don't have additional comments.

@Gui-FernandesBR Gui-FernandesBR merged commit 45971e9 into develop Jul 12, 2024
5 of 7 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/environment-class-major-refactor-may-2024 branch July 12, 2024 23:25
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 Environment Enviroment Class related features Refactor Tests Regarding Tests
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Selected Ensemble members
4 participants