-
-
Notifications
You must be signed in to change notification settings - Fork 194
ENH: adds new Function.savetxt method #514
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
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
72df17a
ENH: Add savetxt method to Function class for saving data to a text file
Gui-FernandesBR 69317c3
TST: add unit tests to cover the savetxt method
Gui-FernandesBR 0247289
DOC: Add export functionality to Function class usage docs
Gui-FernandesBR e3b468d
MNT: updates the CHANGELOG after #514
Gui-FernandesBR 30be2db
Merge branch 'develop' into enh/function-savetxt
Gui-FernandesBR 696e709
ENH: no longer mutates the Function obj when using savetxt()
Gui-FernandesBR File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
andto_csv
methods instead of justsavetxt
?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. Theto_
functions come from pandasThis is just a loose suggestion. I don't mind the current name
There was a problem hiding this comment.
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_
namesThere was a problem hiding this comment.
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
toto_txt
we would be creating an opportunity for confusion since theto_txt
andto_json
would export totally different contents.There's a reason for having the
save
methods to save the source and theto_
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.