-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add writers for xdr and ascii files #40
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.
@vnmabus Thank you for the review! Very useful comments that improved the code. I have addressed the comments and included a few extra tests too.
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.
It is almost ready. Some small comments and questions.
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.
@vnmabus Thank you for the review! I replied to comments and fixed also a NumPy 2.0 compatibility issue.
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.
LGTM. I think this can be merged now. If we have future improvements, they can be done in a different PR.
About the JOSS paper, there is an open discussion in pyOpenSci/software-submission#144 , because usually in order to be fast-tracked, there should be no changes between pyOpenSci acceptance and JOSS submission, and for this package we have made several changes in the middle. My hope is that at least we could submit it using the normal submission procedure (without fast-tracking), but we need to wait for answers as this situation is highly unusual.
@vnmabus Thank you for the thorough review and the update regarding JOSS! |
References to issues or other PRs
Closes #31.
Describe the proposed changes
This PR will add basic writing functionality.
Demo (
demo.py
):Output:
Additional information
Testing
Two tests looping over all test files in the repository have been added:
rdata/tests/test_write.py::test_write
: Test that read-parse-write results in bit-wise unchanged files (omitting compression)rdata/tests/test_write.py::test_convert_to_r
: Test that R-to-Python-to-R conversion results in unchanged RData object.Implemented features are indicated in the test output:
XFAIL
: writing/converting of the given type has not implemented in this PR,SKIPPED
: R-to-Python-to-R conversion might be ambiguous (not enough information in Python object to construct the original R object).Test output (click to expand)
These tests do not cover all the functionality of writer, so extra tests need to be added still.
Documentation
No documentation in README or https://rdata.readthedocs.io is included in this PR yet.
Checklist before requesting a review