ENH: adds new Function.savetxt method#514
Conversation
Gui-FernandesBR
added
the
Enhancement
github-actions
bot
requested review from
MateusStano,
giovaniceotto and
phmbressan
Gui-FernandesBR
force-pushed
the
enh/function-savetxt
branch
from
13fc909 to
e3b468d
Compare
Codecov ReportAttention:
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. |
| ) | ||
|
|
||
| def savetxt( | ||
| self, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Actually, just saw what you commented in the description. I still think it could be more descriptive to use to_ names
There was a problem hiding this comment.
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.
|
All fixed here. |
phmbressan
left a comment
phmbressan
left a comment
There was a problem hiding this comment.
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.
3 participants
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest --runslow) have passed locallyCHANGELOG.mdhas 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
Additional information
to_serializablein other to export different attributes of the Function instances to a .json file. Let's leave this for a future PR.