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

BUG: User input checks added for Function class #451

Merged
merged 21 commits into from
Nov 17, 2023

Conversation

brunosorban
Copy link
Collaborator

@brunosorban brunosorban commented Nov 1, 2023

Enhanced Validation and Default Settings via _check_user_input Method for the Function class. The features added comprise:

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

See issue #443.

New behavior

Type Checking and Conversion for Source Data:
The method now checks for list and np.ndarray the the data is consistent. It uses NumPy conversion to an array of type np.float64 for input validation. This ensures consistent data type handling and error triggering for non-homogeneous or non-numeric arrays.

Output Parameter Validation:
Enhanced checks for the outputs parameter have been added. If outputs is a string, it's converted into a list. Additionally, the method now raises a ValueError if outputs has a dimension greater than 1, enforcing strict dimensionality constraints.

Input, Interpolation, and Extrapolation Handling for Multidimensional Functions:
For sources with dimensions greater than 2:
The inputs are automatically set to ["Input 1", "Input 2", ...] if they were initially ["Scalar"], and a warning is issued to inform the user of this default setting.
The interpolation method defaults to "shepard" for multidimensional Functions, with a warning if any other method is specified, indicating limited support for interpolation methods.
The extrapolation is set to "natural" if not specified, with a corresponding warning for clarity and default behavior.

Source Dimensionality Consistency Check:
The method now verifies the consistency between the dimensions of source and the combined length of inputs and outputs. It raises a ValueError if there's a mismatch, ensuring congruence between source data and expected input/output dimensions.

Function Source Handling:
When the source is a function, the method checks and updates the inputs and outputs to match those of the source function, issuing warnings if there are discrepancies.

These updates aim to make the method more intuitive, user-friendly, and error-resistant, providing more precise feedback and automatic user adjustments. The added validations and default settings should minimize common configuration errors and guide users toward correct usage patterns, particularly in handling multidimensional data and function sources.

Added Argument Count Check:
The code now includes a conditional check to compare the length of args (arguments passed to the function) against the expected domain dimensionality (self.dom_dim).
If the number of arguments (len(args)) does not match the expected domain dimension (self.dom_dim), the function raises a ValueError.
The error message clearly states the discrepancy, indicating both the expected number of arguments and the number provided. For example: "This Function takes 3 arguments, 2 given."

Breaking change

  • Yes
  • No

Additional information

These checks should avoid common user errors, but more can always be added. Please feel free to give feedback so we can improve it even more.

@Gui-FernandesBR Gui-FernandesBR changed the title User input checks added for Function class BUG: User input checks added for Function class Nov 2, 2023
@MateusStano MateusStano added this to the Release v1.X.0 milestone Nov 3, 2023
@Gui-FernandesBR Gui-FernandesBR added the Bug Something isn't working label Nov 3, 2023
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.

Hey @brunosorban very nice job starting this one. I think we are going to the correct target.
I have some comments on your additions, please feel free to ask for further details if needed.

a task list before merging:

  • Different tests are not passing. Maybe you were under the false impression since only the black linter. We need to fix that before proceeding (54 failed, 777 passed, 22 skipped, 35 warnings, 69 errors in 49.32s)
  • Add unit tests to cover the new _check_user_input method
  • Document the _check_user_input (docstrings)
  • Ensure that all input validations are being done inside the new method and there's no redundant comparisons in the rest of the class.

rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
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.

Approved by now. More unit tests could be added to improve the code, but I manages to cover at least the very basic functionality of the new function.

There was another problem in the plot2D function, already solved in my last commit.

To be used in the future:
https://pytest-with-eric.com/introduction/pytest-assert-exception/#:~:text=Exceptions%20can%20be%20tested%20in,doesn%27t%20raise%20an%20exception.

@Gui-FernandesBR
Copy link
Member

As I was the last person to edit the PR and my changes were kinda delicate, I'd like to wait for another review here before merging it into the master branch.

@MateusStano @phmbressan , could one of you review this one? Please don't get biased by my approved review, this one is important for the future releases...

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (433435f) 68.92% compared to head (848c860) 69.50%.
Report is 1 commits behind head on develop.

Files Patch % Lines
rocketpy/mathutils/function.py 87.03% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #451      +/-   ##
===========================================
+ Coverage    68.92%   69.50%   +0.58%     
===========================================
  Files           55       55              
  Lines         8961     8993      +32     
===========================================
+ Hits          6176     6251      +75     
+ Misses        2785     2742      -43     
Flag Coverage Δ
unittests 69.50% <87.03%> (+0.58%) ⬆️

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.

Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

Great checks for input consistency here, but I caught an error in plotting 2D functions due to shepard interpolation that must be fixed before merging.

Requesting changes for now so nobody merges, I will try to fix the Plot2D error meanwhile.

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member

@phmbressan please improve tests so we prevent from getting the same error in the future.

@phmbressan
Copy link
Collaborator

phmbressan commented Nov 17, 2023

I have fixed the plotting error that I mentioned on my earlier review and implemented some tests for the values of shepard interpolation to prevent this from happening again.

I would rather if someone double checked the changes before merging. Could you review these @Gui-FernandesBR ? Also, there are some comments I made in my original review that I did not apply so as not have an unilateral change, I would like to hear your opinion on the multiple outputs lambda function.

After that I will approve this PR and we are good to go.

Gui-FernandesBR and others added 2 commits November 17, 2023 17:34
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
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.

Great job. Overall a good correction

@Gui-FernandesBR
Copy link
Member

@phmbressan please solve your request changes and merge it into develop. We have other branches coming up tomorrow,

@phmbressan phmbressan merged commit 00d1362 into develop Nov 17, 2023
13 checks passed
@phmbressan phmbressan deleted the bug/function-input-validation branch November 17, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

5 participants