-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
ijtiff
#164
Comments
Thank you for your submission @rorynolan 😸 It's a good fit for rOpenSci - and we'll assign an editor asap |
(@rorynolan is there a way to instal ImageJ on osx 10.13 (High Sierra), seems I don't need it to run your pkg, but still curious) |
Editor checks:
Editor commentsThanks for your submission @rorynolan !! Goodpractice output for you and reviewers to consider: It is good practice to
✖ write unit tests for all functions, and all package code
in general. 72% of code lines are covered by test cases.
src/common.c:21:NA
src/common.c:22:NA
src/common.c:59:NA
src/common.c:60:NA
src/common.c:61:NA
... and 196 more lines
✖ avoid calling setwd(), it changes the global environment.
If you need it, consider using on.exit() to restore the working
directory.
R/write.R:37:13
R/write.R:38:5
tests/testthat/test_graphics.R:5:11
tests/testthat/test_graphics.R:6:3
tests/testthat/test_io.R:4:11
... and 18 more lines Seeking reviewers now 🕐 Reviewers: @jeroen @jonclayden |
Hi @sckott, |
ah right! i totally forgot about fiji - i did use it in the past, but forgot. thanks for the help |
still seeking reviewers 🕐 |
Reviewers are @jeroen @jonclayden - Due 2018-01-18 |
ReviewThank you for your submission and for introducing me to ImageJ! ROpenSci is working on improving image processing/analysis tools in R and you are doing interesting work in this space. We should see if we can collaborate more in the future. Main PointsThe readme explains that
Perhaps you should emphasize support for high dimensionality (multi frame) tiff images which I think that this is mainly about? Or am I missing something? ExampleThe readme example image has 2 channels of 5 frames but does not explain what each channel contains. How does the user know what color or channel type the data is? Perhaps a 'real world' visible image data would be more helpful. If we read the example image using the 'magick' package we get 10 frames with 1 channel instead of 5 frames with 2 channels. library(magick)
path_2ch_ij <- system.file("img", "2ch_ij.tif", package = "ijtiff")
image_read(path_2ch_ij) Is this a bug in ImageMagick (ignoring some tiff property) or a custom property of ImageJ to group frames into channels? I am curious if some of ijtiff functionality could also be accomplished with the magick package? The benefit of ImageMagick is that it is not limited to tiff but will work for any image format. For example: library(magick)
path <- system.file("extdata", "bleached.tif", package = "detrendr")
img <- magick::image_read(path)
frames <- lapply(img, image_data) Can you write a little bit in your readme or vignette about what additional functionality ijtiff adds? Small commentsI tried this example image from the ImageJ website: https://imagej.nih.gov/ij/images/Composite.tif. However it gives an error when reading this with ijtiff. Is this expected? Why Make sure you add the missing Package does not build out of the box on Windows as promised. I will send a PR to fix this :) |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3 Review CommentsThis review is based on the package as of commit rorynolan/ijtiff@8bfa0ac. I realise some further commits have been made recently, but I need a stable reference point to base my comments on. Feel free to disregard any comments that no longer apply. The
|
Thanks for your reviews @jeroen and @jonclayden ! @rorynolan reviews are now in. There's no one way to respond to reviewers, but here's some examples of responses to reviewers that we like: |
Responding to @jeroenMain Points
Example
Small Comments
Package does not build out of the box on Windows as promised. I will send a PR to fix this :)
Finally
Thank you for your review and for going to the considerable bother of a PR. I really appreciate it and I think the package is already much improved thanks to you. PS @jonclayden, currently working on a response to your comments. |
Responding to @jonclayden
Thanks a lot for all your comments and suggestions @jonclayden. I really appreciate the time you're putting into this. |
Thanks, Rory, for the quick response. I think the vignette is much clearer and more helpful now. The namespace syntax is really a matter of taste, so if you prefer it as it is then that's perfectly fine. (Hadley slightly contradicts himself, though, as he says to "always" use The working directory issue is odd. Thanks for clarifying what you mean by a far-away directory, but I don't seem to be able to replicate the problem locally (with the relevant part of It's great to see more interest in image processing in R. Thanks again for your contribution! |
(FYI, jeroen is on vacation for another two weeks, so likely won't get a response until then) |
@jeroen are you happy with the maintainers response to your review? any other thoughts/comments? |
1 similar comment
@jeroen are you happy with the maintainers response to your review? any other thoughts/comments? |
Sorry for the delay. The updated readme is a big improvement, it's more clear now what problem exactly the package is solving. I think it's good to go. I'll see if I can figure out what's going on with my 32bit windows libs. |
is the 32bit windows libs thing something we should wait on here, or something for later? |
The author has already pushed to cran, it's fine. |
Yeah sorry for jumping the gun and pushing to CRAN. The reason was that the old package was throwing some CRAN check errors and the CRAN maintainers were bugging me to fix them ASAP. |
Thanks @rorynolan for the udpate. I'll have a quick look over it ... |
Approved! Thanks again for your submission @rorynolan
|
Thank you @sckott for a wonderful review experience!
Finally, can you tell me how the JOSS paper proceeds from here? |
@stefaniebutland ping on the longer form blog post ^^ |
Great, thanks @rorynolan and glad you enjoyed it! For JOSS:
|
@rorynolan Glad to hear the review experience worked well for you and doubly glad you're interested in writing a post. Here are some editorial and technical guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post. Here are some other posts in the onboarding series https://ropensci.org/tags/review/ We ask for a draft to be submitted by pull request a week prior to the publication date so that we have a chance to give you feedback. What do you think of March 20 for draft submission? If you submit earlier, it does give us an opportunity to post earlier if someone else postpones. |
Hi @sckott, |
Hi @stefaniebutland, |
i don't know, is there any timing to consider between approval here and submission to JOSS @noamross ? |
Sounds good! I'll confirm your publication date after receiving your draft. Don't hesitate to ping me here if you have any questions. |
Hi @stefaniebutland, |
Summary
What does this package do? (explain in 50 words or less):
Correctly import TIFF files that were saved from ImageJ and write TIFF files than can be correctly read by ImageJ. Full support for TIFF files with floating point (real-numbered) pixels. Also supports text image I/O.
Paste the full DESCRIPTION file inside a code block below:
https://github.com/rorynolan/ijtiff
[e.g., "data extraction, because the package parses a scientific data file format"]
Data extraction, because the package can import (correctly) a common type of TIFF file that is not currently well-supported in R.
R users who also use ImageJ, or who collaborate with people who use ImageJ.
yours differ or meet our criteria for best-in-category?
There's another R package for TIFF I/O, but it does not interface well with ImageJ (which I would argue is very important). It's also not actively maintained (I'm not getting responses when I file an issue).
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
[ y] Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:[ y] Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
Andrzej K. Oleś (aoles)
The text was updated successfully, but these errors were encountered: