Add MRC file writing support#147
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 87.78% 88.20% +0.42%
==========================================
Files 5 5
Lines 753 814 +61
Branches 99 107 +8
==========================================
+ Hits 661 718 +57
Misses 56 56
- Partials 36 40 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Looks good at first glance but I am not an MRC expert. I'll see if I can get someone else to also have a look.
In the meantime, please also
update your branch with the latest changes on master(just did that)- add yourself to AUTHORS
- add an entry to CHANGELOG
- check your coverage and add tests where necessary
|
@marinegor do you have some experience with MRC files and would you be able to have a very brief look at this PR? |
marinegor
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR!
I've added couple of comments regarding self.header vs self._mrc_header, and regarding using with open(...).
Also, perhaps it's a good idea to add a small test MRC (say, 4x4x4 with random numbers) file with/without a header that tests things? This way we can actually track regressions if something changes in mrcfile itself, while a roundtrip test won't check that.
orbeckst
left a comment
There was a problem hiding this comment.
Some minor fixes required and please address @marinegor 's comments. That should also help with test coverage.
orbeckst
left a comment
There was a problem hiding this comment.
Sorry it took me a while to get back to the PR. Thank you for addressing all remaining issues and improving the tests. This is good to go and I'll merge.
|
@marinegor could you please have a quick look and confirm that all your concerns have been addressed? Thanks! |
|
@marinegor , to my reading, all your comments have been addressed. I'll wait until tomorrow if you have any comments or want to explicitly approve but I also understand that you're quite busy and might not have the time to come back, in which case I'll dismiss your review and merge. |
Head branch was pushed to by a user without write access
Reviewer's comments have been addressed but reviewer has been to busy to re-review.
|
Thank you for your contribution @Pradyumn-cloud ! 🎉 |
Fix Issue - #108
Add MRC file writing support