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: adds new Function.savetxt method #514

Merged
merged 6 commits into from
Jan 21, 2024
Merged

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Dec 17, 2023

Pull request type

  • Code changes (bugfix, features)

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
  • CHANGELOG.md has been updated (if relevant)

Current behavior

If you have a Function object, good luck trying to export this to be used later or to send to someone else.

New behavior

Inspired by the numpy.savetxt method, I've created the Function.savetxt that is currently working very well for both 1D or N-D Function, with or without discrete sources.

Breaking change

  • No

Additional information

  • In the future we may need a to_serializable in other to export different attributes of the Function instances to a .json file. Let's leave this for a future PR.
  • The tests are running really fast... thank you numpy for exsisting.
  • Also, can I say how better the Function classes are now after we officialized the support for csv files with headers? Amazing feature!

@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Dec 17, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Dec 17, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Dec 17, 2023
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner December 17, 2023 20:58
Gui-FernandesBR added a commit that referenced this pull request Dec 17, 2023
Copy link

codecov bot commented Dec 17, 2023

Codecov Report

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

Comparison is base (2205fdf) 72.20% compared to head (696e709) 72.23%.
Report is 1 commits behind head on develop.

Files Patch % Lines
rocketpy/mathutils/function.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #514      +/-   ##
===========================================
+ Coverage    72.20%   72.23%   +0.02%     
===========================================
  Files           56       56              
  Lines         9376     9389      +13     
===========================================
+ Hits          6770     6782      +12     
- Misses        2606     2607       +1     

☔ 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.

Really good PR, complete and with a nice set of tests. There is one detail that I believe is important to be looked upon: the set_discrete changes the source.

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
@@ -2817,6 +2817,76 @@ def compose(self, func, extrapolate=False):
extrapolation=self.__extrapolation__,
)

def savetxt(
self,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of having to_txt and to_csv methods instead of just savetxt?
This would make sense if we were ever to add a way to save a function to a .json for example, with to_json

It seems you got savetxt from numpy. The to_ functions come from pandas

This is just a loose suggestion. I don't mind the current name

Copy link
Member

Choose a reason for hiding this comment

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

Actually, just saw what you commented in the description. I still think it could be more descriptive to use to_ names

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an interesting suggestion.

Between numpy and pandas, I have to say that numpy might be a better inspiration in this case, since pandas is not one of the rocketpy's dependency.

Beyond that, I believe we need to be careful with the to_ methods, let's see:

  • .savetxt: takes a Function instance and save its source to a text file, where each row represents one point from the source.
  • .savecsv: this call the savetxt method above but modify the file extension to .csv
  • .to_json (or other similar name like ``) takes the Function instance and create a json file with different attributes from the object (e.g. interpolation method, inputs and outputs names, repr method, etc.)

If we change the savetxt to to_txt we would be creating an opportunity for confusion since the to_txt and to_json would export totally different contents.
There's a reason for having the save methods to save the source and the to_ methods to convert the entire object to something else.

Let me know in case your disagree with this.

The #522 might be connected to this comment too.

@Gui-FernandesBR
Copy link
Member Author

All fixed here.
@MateusStano @phmbressan could you please re-review it?
Thank you for the past comments btw, I'm happy that you brought my attention to certain points.

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.

As I mentioned in my last review, really good, this PR makes it really easy to export data in RocketPy. Nice to see that #519 was indeed useful.

@Gui-FernandesBR Gui-FernandesBR merged commit 5ced96d into develop Jan 21, 2024
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/function-savetxt branch January 21, 2024 19:20
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
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants