-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added netcdf #273
Added netcdf #273
Conversation
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.
Some minor changes in the code. Would you also please add some tests?
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.
Please add instructions to the readme that list how to output either csv, netcdf, or printing the csv to the screen
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.
If you run this
music_box -e Analytical --output-format netcdf
output is printed to the screen as csv data. There is not a printable netcdf format since it's a binary format. So, when the output is netcdf and no output path is given, I think we need to output anyway. Choose a default filename (maybe results
, this would also close #237) and set the suffix to .nc or .csv based on the output format. What do you think?
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.
Looks good! I just have a minor request but it shouldn't prevent your PR from being merged.
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.
nice!
9e9b090
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.
looks good!
CMD argument is available to define type of file.
Units have been added to csv file header
units added to header in netcdf
close #238 and #237