RocketPy

RocketPy copied to clipboard

Pull request type

• [x] Code changes (bugfix, features)

Checklist

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

Current behavior

Different datasets present undesired values that we usually have to remove manually.

New behavior

The Function class now can remove outliers from the source (if array like) using the IQR method.

Breaking change

• [x] No

Additional information

• This was saved in my machine for a while. It is simple but has been working without problems.
• It will not be included in the version v1.2.0, but in the 1.3.0 instead.

Feb 12 '24 17:02 Gui-FernandesBR

Codecov Report

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

Project coverage is 72.62%. Comparing base (bb65a33) to head (fda6b33). Report is 16 commits behind head on develop.

@@             Coverage Diff             @@
##           develop     #554      +/-   ##
===========================================
+ Coverage    72.60%   72.62%   +0.02%     
===========================================
  Files           59       59              
  Lines         9584     9598      +14     
===========================================
+ Hits          6958     6971      +13     
- Misses        2626     2627       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Feb 12 '24 18:02 codecov[bot]

Interesting feature, useful for removing spurious points in data.

However, the way this method parameters were set (kwargs that change based on another parameter) seems to overcomplicate its usage in order to prevent breaking changes. For instance, I see the following disadvantages:

• I see that you explained everything in the documentation, but it has the potential of getting quite convoluted if we add more types with different **kwargs names.
• Code completition in most IDEs will not be available, since the parameters do not have a fixed name.
• It is quite hard use the method without reading its documentation.
• We can add, if needed, the kwargs behavior later without a breaking change, but we cannot remove or change it.

Do you have any references for this, or any particular reasoning I might have missed? I recall solve_ivp doing something similar, but in their case the method overall has a common set of parameters and the optional ones are generally secondary.

Personally, I would either standardize the parameter names among the different types or make them standalone methods.

I understand your points.

Just to check on this first:

• Are you okay with us having multiple methods to perform the same operation but with some different approaches?
• I am sorry but I cannot anticipate what will be the required parameters for eventual future new methods.
• References? Matplotlib does this quite a lot. Actually, they often do not care about describing the kwargs at all: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.plot.html

Feb 22 '24 21:02 Gui-FernandesBR

I understand your points.

Just to check on this first:

• Are you okay with us having multiple methods to perform the same operation but with some different approaches?
• I am sorry but I cannot anticipate what will be the required parameters for eventual future new methods.
• References? Matplotlib does this quite a lot. Actually, they often do not care about describing the kwargs at all: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.plot.html

Great questions:

• I am ok having multiple methods as smaller and less coupled chunks of code is generally considered a better practice;
• That is completely normal, sometimes we cannot predict future argument structure;
• The references thing was just to understand if you had any particular implementation in mind or whether I did not foresee a possible common practice in this situation.
  • About the link you mentioned, I don't think this is exactly the same as the issue we are discussing: matplotlib **kwargs do not change depending on the value of the others mandatory parameters, i.e., all of the kwargs are meaningful;
  • As I mentioned, the issue for me was not the mere existence of kwargs, but the possibility that they change based on the selected outlier detection string.

Feb 23 '24 12:02 phmbressan

I understand your points. Just to check on this first:

• Are you okay with us having multiple methods to perform the same operation but with some different approaches?
• I am sorry but I cannot anticipate what will be the required parameters for eventual future new methods.
• References? Matplotlib does this quite a lot. Actually, they often do not care about describing the kwargs at all: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.plot.html

Great questions:

• I am ok having multiple methods as smaller and less coupled chunks of code is generally considered a better practice;

• That is completely normal, sometimes we cannot predict future argument structure;

• The references thing was just to understand if you had any particular implementation in mind or whether I did not foresee a possible common practice in this situation.

  • About the link you mentioned, I don't think this is exactly the same as the issue we are discussing: matplotlib **kwargs do not change depending on the value of the others mandatory parameters, i.e., all of the kwargs are meaningful;
  • As I mentioned, the issue for me was not the mere existence of kwargs, but the possibility that they change based on the selected outlier detection string.

OK! I will be refactoring this implementation as soon as possible

Feb 23 '24 22:02 Gui-FernandesBR

All comments addressed, @phmbressan could you review this one again?

Feb 26 '24 19:02 Gui-FernandesBR

Thanks for the solid review, @phmbressan . All comments addressed again, could you take a look and approve?

Feb 27 '24 18:02 Gui-FernandesBR