-
-
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
Submission: tacmagic - PET Analysis in R #280
Comments
thanks for your submission @eebrown - @melvidoni has been assigned as editor |
Editor checks:
Editor commentsThank you for your submission @eebrown! The output from goodpractice is perfect. I will start looking for reviewers.
Please add the badge to your repo. Edit the code below with your issue ID:
Reviewers: Jon Clayden (@jonclayden) & Brandon Hurr (@bhive01) |
Thank you very much @melvidoni. I've added the badge. |
Reviewers have been assigned: Jon Clayden (@jonclayden) & Brandon Hurr (@bhive01) |
@eebrown, Is there a way to get more example files than the built in ones? I don't have a PET device to pull from. I just want to throw as much at it as I can. |
Hi @bhive01, thanks a lot for agreeing to review this package! You've probably seen the data in inst/extdata, with 3 participants in .tac/.voistat format, 1 in magia's .mat format and sample data in DFT. Off hand I don't have any more publicly available analyzed data for testing but I may be able to generate some more for you. |
Hi again @bhive01. I've found some .tac and .dft sample files sample_tac_files.zip from the open source PET analysis C library from the Turku PET centre (www.turkupetcentre.net). I have had to make some minor changes to the tac loading functions to support some idiosyncrasies with the files, so I have updated the devel branch with that. I am not sure what the best practice is for updating the master branch while under review, but I can merge in these changes if that's best (https://github.com/eebrown/PET/compare/devel) Please let me know if there is something else you were hoping to test more. |
@eebrown If the changes are not too fundamental (only for the examples), you can then merge the changes once you have received the reviews. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 7 Review CommentsFirstly, while I am very much an image analyst, "I don't do humans" (Ace Ventura 1994), so I'm a bit out of my element with PET image model outputs. For that purpose, and as far as I can tell, this package works as intended and provides meaningful data processing and plots for examining PET data output in the various formats built-in (PMOD TAC, Voistat, DFT, Magia/Matlab). Below are the outputs from goodpractice, devtools::check, and devtools::test. Overall there are a few issues found by goodpractice::gp around unit testing, but coverage is pretty good.
Function NamingYou have functions named after authors of papers (Logan and Aiz). I guess this is a good idea because it tells you where they came from, but perhaps they could have more generic names so in the future you could use a "method" argument if there are other methods added in the future. This suggestion comes from a place of naiveté so cum grano salis.
Some inconsistencies in the naming convention used for functions with the use of CamelCase. https://ropensci.github.io/dev_guide/building.html#function-and-argument-naming suggests using snake_case which is mostly used. I would update the following:
Where the naming is applied it’s clear from the name what the function is responsible for.
|
Dear @bhive01, thank you very much for your thoughtful thorough review and very helpful suggestions. I look forward to incorporating them into the package and will get back to you here. |
Thanks for checking in @jonclayden--and thanks for fitting this review into your schedule. I look forward to it! In the meantime I will be addressing the suggestions by @bhive01 but will keep them on the devel branch. |
Just want to say thanks for writing the package @eebrown. I think it does what it says on the tin pretty well. I also want to say that you should take many of the suggestions as just that. If they make sense apply them when you can, if they don't, don't worry about it. |
Response to @bhive01Thank you again for taking the time to thoroughly review the package. I have implemented many of your suggestions on the devel branch as indicated below. I noticed you skipped the JOSS section -- I am not sure if this is done later in the review, but I would like to submit to JOSS after the reviews.
[...]
I am not sure what to make of the WARNING and ERROR you see with goodpractice, as I don't get those errors on my Ubuntu or macOS machine.
Thanks for pointing this out. I've now renamed the above functions to be consistent with the naming guidelines and the rest of the package's functions. As for the author-named functions, I think I will leave as-is currently, as I could always add a more generic function after (but harder to go other way around, and it's not yet clear what parameters the generic function would need). Commit f1ac29e.
Thanks. Fixed in d49a41a.
I like the idea of making use of the file extensions in data loading. I think it is a balance of convenience vs. code safety and I think for now it makes sense to err on code safety. So instead of assigning the format from the file extension, I have added the file extension as a check that now warns the user if there is a mismatch between their specified file type versus the file extension. Commit b03af39.
Agreed. Commit c9ded13.
I made a function called copy_tac_attributes() to get the specific attributes from an original tac object and assign them to the copy. This will make things more convenient if more attributes are needed in the future. Commit 434b37c.
I have moved the plotting functions into plot.R as suggested. As for the warnings about plotting too many ROIs, I think it may not be necessary as it would be self evident. I see the plotting function currently as a quick way to visualize tacs when performing analyses rather than for the generation of publication-ready plots, but this is obviously an area that could be developed. I think at that point it will make sense to overhaul the visuals including a better colour scheme, among other optimizations. Commit 7d2b4b1.
I created a new function validate_suvr_params() in suvr.R that handles the checks that were needed and is now used in both suvr() and suvr_auc(). Commit 6806094.
Done as previously noted.
Fixed. Commit efe3250.
I have now added a runable example to the vignette. I was avoiding it, because the batch functions rely on real-world file names and directory structures but I have hopefully made it clear how it would be different in practice. In the process I also added a new function that is useful when PMOD tacs are loaded but when combining ROIs is not desired: split_pvc(). It is useful for PMOD tac files that have both PVC-corrected and uncorrected ROIs but when only one copy is needed. It is in the tac.R file and documented. Commit 629712a. Rebuilt vignette commit b9ce86d.
Yes this is for consistency with PMOD files.
Yes the PMOD tac files don't have volume information and saving is currently built around PMOD .tac files.
Thanks :)
Please let me know if you have any questions/concerns about the changes. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsThanks for the opportunity to review this well-constructed and well-documented package for analysing time–activity curve (TAC) data derived from PET. The package appears to follow the requirements of the rOpenSci packaging guidelines, (NB: I have not read the other review, in the interests of objectivity, so feel free to disregard any comments that have already been addressed!)
|
Thank you very much @jonclayden for the thorough review and great suggestions. I will address each and report back here. |
Response to @jonclayden, Review 2
Thanks for the suggestion; I have tried to clarify this in the background of the vignette, along with updates that take into account the rest of your feedback below. [devel b88c9e6]
I think this makes sense and will help with future expansions. I've used the "tac" class and changed plot_tac to a plot.tac method, and added a print.tac method for a quick summary. [devel f3d8fe3]
The other reviewer also suggested expanding the colour options. I have added the ability to specify a colour palette. [devel 4d0f181] [devel 0d98a26]
This was a good suggestion as the code is now cleaner using the ellipsis argument, and also more flexible for custom models and future expansion. [devel 6728c31] Relatedly, I added a validate_ref_Logan_params() function to ensure proper parameters are supplied. [devel 0c1ffb1]
The problem is that unlike SUVR, the Logan method is just one model of DVR, and furthermore there is the reference-region non-invasive version of the Logan graphical analysis and another version, not implemented here, that uses arterial blood sampling. So the function names appear verbose but that is to disambiguate them from other models and I think this specificity is required. The DVR_all_ref_Logan() runs the model on all ROIs, but having the DVR_ref_Logan function that takes just 1 target can be quite helpful for testing out the model, e.g. to find an optimal t-star or explore data where the model has issues.
SUVR does not need plots like the Logan graphical DVR method, but I take your point. I now have DVR_ref_Logan(), the main DVR function, outputting a list object of the calculated model and related variables as a ref_Logan class, which can be plotted with a plot() method, plot.ref_Logan(). [devel 5e169fc]
Thanks, I did not know about the handy @references tag. [devel cd1fecf]
Thanks for catching this. [devel 82b7eed]
Thank you for noting these. [devel 6c50b73] [devel 8471eef] Additionally, I have added some tests to cover some of the new changes. [devel fab5007] Thanks again for the review! |
Thanks for the response, Eric. I will check out the updated package shortly, but in the meantime, I just want to follow up on my point 5. I understand the desire to allow only one target, and to allow for the possibility of new fitting methods in the future. But my suggestion was to use a single function with a signature along the lines of dvr <- function(tac_data, target=NULL, ref=NULL, method="logan", k2prime=NULL, t_star=NULL, error=0.10, int_method="trapz", params=NULL) which would allow calls such as # like DVR_ref_Logan()
AD06_DVR_fr <- dvr(AD06, target="frontal", ref="cerebellum", k2prime=0.2, t_star=0)
# like DVR_all_ref_Logan(); target=NULL, the default, implies all
AD06_DVR <- dvr(AD06, ref="cerebellum", k2prime=0.2, t_star=23) while also improving semantic consistency and still allowing other models and/or fitting methods to be added later. In the end I don't feel that strongly, and by all means leave it as it is, but I just wanted to make sure that it's clear what I had in mind. |
Thanks for clarifying. I can do this and it makes sense. In the future I may have to switch to the ellipsis rather than the specific parameters as other DVR models need different parameters. |
Sure, that makes sense – you could keep Everything else looks good, although you may want to implement an indexing method, |
I changed print.tac to summary.tac because it seems to fit better and keeping print as the fallback data.frame style print output works better with other generic functions. I also created an as.tac() function to create tac objects from data frames, and added an explanation to the vignette. [devel f5cb2f9] I added the dvr() wrapper function with tests and updated the vignette. [devel 578a89b] |
Looks good! Thanks for being so receptive – I hope the process has been helpful. I'll mark my approval on the review. |
Thank you @jonclayden. Please, let me know what you think, @bhive01. |
I have marked it for approval as well @melvidoni . Thanks @eebrown for taking onboard so many suggestions in a short period. It was a lot of work I'm sure. |
Approved! Thanks, @eebrown for submitting and @bhive01 and @jonclayden for your reviews! To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing. We've started putting together a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
Thanks very much @melvidoni. This has been a fun and useful process (with great international collaboration!) I will make the needed badge changes and also would like to increment the package version to 0.2.0 given the number of changes implemented during the review. Do I then proceed with the JOSS submission at that point? I would love to acknowledge the reviewers as rev contributors in the description. Please let me know if you consent to this @bhive01 and @jonclayden. (And please let me know your ORCID if you would like that included). |
@eebrown Doing the JOSS submission is not mandatory. That is only if you want, in which case, it should have been stated in the opening post. Also, you tagged @jonclayden incorrectly, so please Jon, read the comment before this! |
Hi @melvidoni, sorry for the confusion--I think I did indicate that I wanted to submit to JOSS in the original post (it is checked) and I did included a paper.md and bib file for this purpose. Please let me know if I missed something so I can fix it if possible. |
I reviewed the package on the basis of it being submitted for JOSS, and have ticked off those points in my review. Many thanks @eebrown for the offer of the acknowledgment, which I'm more than happy to consent to. My ORCID is 0000-0002-6608-0619. |
@eebrown I don't know why, but I don't see the box ticked on the opening post for the issue. Nevermind this, I apologize for the misunderstanding. I have added three additional to-dos on the approval comment and I need you to tackle those as well. Do everything before submitting to JOSS, please. You should have already received an invitation for the team. |
I also consent to the inclusion as a reviewer. My orchid ID is 0000-0003-2576-4544. |
Hi @melvidoni, I've done all the post-review steps up to the Zenodo part. In Zenodo, I see the repositories under my name but not the ropensci tacmagic repository, so I have not been able to turn on tracking for it yet. I am not sure whether that's something I would need admin status for, if I don't already have that. Thanks for your help. |
@eebrown I'm rOpenSci's Community Manager, here to say we would love to feature a post about This link will give you many examples of blog posts by authors of peer-reviewed packages so you can get an idea of the style and length you prefer: https://ropensci.org/tags/software-peer-review/. Here are some technical and editorial guidelines for contributing a blog post: https://github.com/ropensci/roweb2#contributing-a-blog-post. Please let me know if you're interested and we can discuss a deadline. Happy to answer any questions. |
@eebrown did you transfer Please, remember to tick the boxes on my approval comment each time you have completed something. |
Thanks very much @melvidoni; it works now, and a DOI has been assigned. On the other hand I don't seem to be able to edit your comment, so I've copied the post-approval checklist below. Essentially I am at the last step, about to submit to JOSS. As I would also like to submit to CRAN, does the order of submitting to JOSS/CRAN matter from your perspective?
|
Hi @stefaniebutland - thanks for getting in touch! I think a blog post is a great opportunity to get some eyes on the new package. I'd love to connect by email to hear more: eb@ericebrown.com. |
Thanks, @eebrown, now:
If everything is OK, I am closing the issue. |
Submitting Author: Eric Brown (@eebrown)
Repository: https://github.com/eebrown/PET
Version submitted: 0.1.9
Editor: Melina Vidoni (@melvidoni)
Reviewer 1: Jon Clayden (@jonclayden)
Reviewer 2: Brandon Hurr (@bhive01)
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Please see the presubmission inquiry as this package was deemed in scope.(#277)
This packages supports loading positron emission tomography (PET) time-activity curves (TACs) from several different major formats, to allow a common subsequent analysis pipeline within R ("data extraction"). There are functions to support merging of regional time-activity curves, an often important analysis step ("data munging"). Additional analysis is available in the package, which virtue of being open source promotes open science reproducibility.
Scientists working with PET data needing to analyze TAC data in R can take advantage of the loading (including batch loading) functions and basic plotting. Those who need basic non-invasive models can take advantage of the implemented models. We will invite contributors to extend the package in its support for file formats and models.
There do not appear to be packages that offer the loading and merging functions. There are packages that have some PET models implemented but not full overlap.
Issue #277 - @sckott
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: