Skip to content

Function Comparison and Identity Map #353

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

Merged
merged 16 commits into from
May 18, 2023
Merged

Conversation

phmbressan
Copy link
Collaborator

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

The Function class currently lacks basic methods of comparison and identity map.

What is the new behavior?

The comparison works in a very similar way to comparing arrays in numpy. When this comparison cannot be performed, an error is raised. The identity map is basically an implementation of the function $f(x) = x$ that follows the same discretization as the instance.

Does this introduce a breaking change?

  • Yes
  • No

@phmbressan
Copy link
Collaborator Author

phmbressan commented Apr 19, 2023

It seems that an atmospheric file is not available due to server clutter, then the tests do not pass. It will likely return shortly.

@phmbressan phmbressan force-pushed the enh/Function-Arithmetic branch from dc137b4 to a9f746d Compare April 20, 2023 11:16
@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Apr 21, 2023
@phmbressan
Copy link
Collaborator Author

One particular issue regarding integralFunction:

  • If the lower and upper bounds are the same (which might happen on edge cases), the resultant integral is computed, but cannot be turned into a Function because the spline interpolation gives an error;
  • linear interpolations work well, which might be an issue for I believe the behavior of the method ought be the same no matter the interpolation type.
  • For reference, try building the Function([(-0.6,0),(-0.6,0)]) and Function([(-0.6,0),(-0.6,0),(-0.6,0)]) with spline and linear interpolations.

Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Just two final comments:

  • Like min and max are cached properties, perhaps so should xArray, yArray, xinitial, xfinal, etc be.
  • It worries me that some new methods, such as min and max, have no handling of 2D Functions. They are rarely used for now tho, so this is not a deal breaker.

Comment on lines +638 to +639
self.setInterpolation(self.__interpolation__)
self.setExtrapolation(self.__extrapolation__)
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember discussing this, but I may have forgotten.

From what I see, we have to different behaviors:

  • Keeping interpolation and extrapolation unchanged when discretizing.
  • Using the interpolation and extrapolation of the model Function.

Could you point out the pros and cons of each? How would the first option work in case self originally has a callable source?

Feel free to keep the proposed solution if you find it the best option. However, make sure to document this behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both ways are possible, however, during coding with this method, I found that keeping the self properties is usually more useful:

  • In general, the Model Function was used only to ensure the same discretization as other in order to perform arithmetic operations. Therefore, the main point was to make the xArray equally discretized;
  • Due the fact that the arithmetic operations now no longer require the same interpolation for array computations, it seems unnecessary to force self into the same interpolation of the model Function. That may cause issues if an "exact" extrapolation (such as linear (i.e. natural)) is substituted by an "inaccurate" extrapolation (such as polynomial)".

As an example of the latter, the IdentityMap is based on accurate linear extrapolation, is one discretizes it based on a model Function of another kind (e.g. something really common) great inaccuracies may appear.

@giovaniceotto giovaniceotto mentioned this pull request May 11, 2023
10 tasks
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
@giovaniceotto
Copy link
Member

giovaniceotto commented May 12, 2023

Here is a to-do list before merging this PR:

  • Resolving conflicts
  • Resolve keeping self vs other interpolation and extrapolation properties
  • Resolve making xArray, yArray, xinitial, xfinal, etc as cached properties

The last two are suggestions, thus optional. @phmbressan let me know if you prefer to skip them.

@MateusStano MateusStano merged commit c0ea88d into beta/v1.0.0 May 18, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/Function-Arithmetic branch May 26, 2023 00:53
@phmbressan phmbressan restored the enh/Function-Arithmetic branch May 27, 2023 22:14
@Gui-FernandesBR Gui-FernandesBR deleted the enh/Function-Arithmetic branch June 14, 2023 09:23
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.0.0 milestone Sep 7, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants