[REVIEW]: PsychoPhysioPipeline about joss-reviews HOT 25 CLOSED

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

• Please work through the checklist at the start of this issue.
• If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
• Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

Please note #40 looks to be a related submission. It may make sense to have both of these reviewed by the same individual.

from joss-reviews.

@openjournals/joss-editors - any suggestions for potential reviewers here?

from joss-reviews.

I got here via accepting review for #40. A quick look suggests that more exposition is needed (explaining terms, clearly defining the broader utility and audience), and much of this would be common across #40 and this submission.

@arfon - are there specific guidelines on what should be submitted separately and what combined? These two submissions are clearly distinct code bases, but they are closely inter-linked and the requirements of JOSS for statement of need would seem to be highly overlapping.

from joss-reviews.

@arfon - are there specific guidelines on what should be submitted separately and what combined? These two submissions are clearly distinct code bases, but they are closely inter-linked and the requirements of JOSS for statement of need would seem to be highly overlapping.

Not really @davclark but this is a good question. @sbogutzky is there are reason why this and #40 were submitted separately?

from joss-reviews.

Yes, a have some reasons:

1. You can use both tools independently
2. The PPC will be used while mobile activities; The PPP will be used stationary
3. Different authorship in both projects
4. Programming language is different

@davclark - which terms need more exposition? The audience is the flow research community e. g. European Flow Researchers Network (EFRN)

from joss-reviews.

Those seem like good reasons @sbogutzky - I'll start with #40, then, as I feel that's where I likely have a bit more specific expertise. Lots of folks could easily comment on an analysis pipeline.

@davclark - which terms need more exposition? The audience is the flow research community e. g. European Flow Researchers Network (EFRN)

Again, I'll restrict to #40. Feel free to apply relevant comments there to this submission, of course! The clarification of audience is helpful - in both cases, you aren't writing to hacker / programmer types, so you should be clear about the skillsets required to use your tools! I'll repeat this in a more formal review on #40.

from joss-reviews.

@davclark - I will add some of your recommendation of #40 to this submission tomorrow, too.

from joss-reviews.

@davclark - Thank for your recommendations. I'm ready with my revision.

from joss-reviews.

I linked the repo to Zenodo:

from joss-reviews.

Editors, please also note that change the title to "PsychoPhysioPipeline: A Processing and Analysis Pipeline for Psychophysiological Research"

It is possible to change that on the Paper page?

from joss-reviews.

I'll plan to have a look at this next week. If someone else wants to grab this, you are most welcome to :)

from joss-reviews.

Well, I lied a little. But better late than never. Note that I'm looking at the updated paper.md in the repo.

First a few small suggestions, not necessary for acceptance, but suggested:

1. Many R users don't know how to build and install packages. I could install flow with R CMD build flow and R CMD INSTALL flow, but package dependencies are not automatic this way. So, you could instead install devtools and use their build() and install() functions, which do automatically install your dependencies. I'd document the latter approach.
2. More generally, why not put everything in the flow package proper? Your scripts could easily become functions.
3. "Source directory" is a strange name - it could mean where the source code is. I'd say "source data". Also, this is brittle - if you don't include a trailing '/', the system breaks.
4. Location of outputs is given in the output of the script - this could be given in the docs (the README). Outputs are self-documented, but no formal docs.
5. "Next" prompt in subset script is confusing. Why is it even there?
6. Docs could explain what Kubios HRV is / where it comes from, and what RR is.
7. Tests would be nice

Things that need a little work

1. Explain that the subset script is a GUI selection tool? More critically, it is not clear to me how I would do the obvious thing - selecting along an x range. The zooming in clips the data along the y range.

2. There's no explicit description of community/support

3. The pipeline is a bit muddled - at first glance it seems like fairly basic R scripts. The real meat seems to be in the flow package, but this submission draws the reader's focus to the R scripts in processing, which I see more as examples of how to use the flow package.

   @arfon do you have any guidance around how submissions to JOSS should coordinate true libraries with supporting / example scripts?

4. I'm going to take the perspective of a lazy person here. Since this is git, I can compare my results to your outputs with a git diff. But then I see differences which I need to visually diff like so:

   -4.3,5,3.25,1,5,7,5,5,2016-03-14 18:18:53,"Running",0,1754583,1839311,1,"Bogutzky","Simon","1984-02-23"
   +4.3,5,3.25,1,5,7,5,5,2016-03-14 18:18:53,"running",0,1754583,1839311,1,"bogutzky","simon","1984-10-31"
   

   The issue there is that I didn't capitalize things, as I believed I needed to match the case of the directories (though apparently not). So, barring explicit tests, some instructions to perhaps delete the processed-data directory, and also about what explicitly to type prior to a git diff (which should result in no difference) would be good.

   Coincidentally, I randomly said you were born on Hallowe'en of 1984. I got the year right 😮

5. DOI for Hreljac2000? doi:10.1016/S0966-6362(00)00045-X

6. The only text / code for KubiosHRV is in the README. It's not clear how I do this.

7. In get-stride-data.R, I have no intuitions about an appropriate threshold for "not detection" (also, is this "knot" detection?). So, I said 5. This yields lots of points around 150 strides/minute. Sounds fast!

8. Also in get-stride-data.R I let the thing sit for a while. It was not very clear that the plot wanted interaction. I also have no idea what I'm doing. After that, there are 265 plots where I can click on things and I don't know why...

At this point, a bit over an hour had passed, and I had exceeded my threshold for digging around.

As-is, I don't think this is up to JOSS standards. Overall, more context is required to understand what the scripts return to me and expect me to interact with. More explicit testing instructions (ideally automated) would go a long way.

It seems like a useful piece of software (and an interesting project), so I encourage @sbogutzky to make such changes. Or alternatively, identify a reviewer with more expertise in this domain (~ sports physiology).

from joss-reviews.

@davclark -- Thanks again, unfortunately I'm a little bit busy, because of the revision of my PhD Thesis. Maybe I can make some changes on the weekend.

Some remarks:

For 1.) The visualization at at the end is only for checking the segment and its data. The range of each segment of data based on the times of self-reports in self-report.csv
If you have a session with motion data and 4 self-reports at minute 10' 20' 30' and 40', you will get four segments of data.

For 3.) The real meat is the pipeline. In the flow-package are only utility functions.
The pipeline produces from self-reports, raw ECG and motion data psychological, physiological and locomotory indicator, which can be used to find correlations between these branches.

For 4.) What is the issue?

For 5.) The doi is right! http://www.gaitposture.com/article/S0966-6362(00)00045-X/abstract

For 7.) The pipeline use the x rotation in deg/s to detect the midswing event of the gait cycle (step)?
200 is a good value.

For 7 and 8.) Ok, there are more instructions needed. For you: You have 265 (maybe) wrong step detection and has to control it -- Remove wrong detections and add missing detection. For few misleading detections, you can use the cloud plot (plot before the control plots) to remove outliers.

from joss-reviews.

JOSS seems pretty relaxed about the review process, and I'm not in any hurry. And, I'm confident you can get this in shape, as none of my concerns are core to your project. For now if you need to put something on a CV, it's fair to say you're "in revision".

To respond to your query about (4) - there should be a reliable way (i.e., the user shouldn't have to think too hard) to test the pipeline. The way it's set up currently, the results of running the pipeline on your example data still incorporate whatever random values I type in. So, my results can differ from the (presumably) correct outputs included in the test data. Usually when testing, "different" == "wrong".

I showed some examples where the results are "differerent" but not in a meaningful way. In this particular case, you could suggest the values to type... probably in general you could suggest values for all of the steps? Then, you'd be able to check that the system is working properly with a git diff, and checking if files have a more recent modification date. Still not great for automated testing, but I'd call that the bare "good enough" - and it could pave the way towards automated testing.

from joss-reviews.

@arfon do you have any guidance around how submissions to JOSS should coordinate true libraries with supporting / example scripts?

Thanks for the review @davclark. I'm afraid I don't fully understand your question 😕. If you're asking whether we expect libraries (or utility functions as @sbogutzky describes them here) to be exercised in the tests then ideally yes. Quoting from the http://joss.theoj.org/about#reviewer_guidelines

Tests

Authors are strongly encouraged to include an automated test suite covering the core functionality of their software.

In addition we have some reviewer guidance about making sure there are examples of how to actually use the software:

Example usage

The authors should include examples of how to use the software (ideally to solve real-world analysis problems).

As I said earlier though, I'm not sure I've exactly understood your question @davclark.

from joss-reviews.

@arfon - I am pretty sure I didn't ask clearly. The test question was a separate thing, and I think I understand that clearly enough, and have communicated as much in my comments.

Right now, this software submission contains an R library (flow), in addition to analysis scripts which @sbogutzky considers the primary contribution. This is awkward, but I wasn't going to force him to separate this into two repositories. I was asking for editorial advice only on this issue.

from joss-reviews.

Right now, this software submission contains an R library (flow), in addition to analysis scripts which @sbogutzky considers the primary contribution. This is awkward, but I wasn't going to force him to separate this into two repositories. I was asking for editorial advice only on this issue.

@davclark gotcha, thanks.

The pipeline is a bit muddled - at first glance it seems like fairly basic R scripts. The real meat seems to be in the flow package, but this submission draws the reader's focus to the R scripts in processing, which I see more as examples of how to use the flow package.

As you point out in this comment, by combining these two components this makes it somewhat more challenging to provide clear documentation for the user and tests/examples to demonstrate how to use the tool. All that said, I don't think we should force the authors into separate repositories provided they can clearly document how to use the tool.

from joss-reviews.

I did a revision.

What I couldn't provide:

• One library for all code - or a separation of the code
• Automated test suite

I made the most changes in the documentation (README.md). I provide all input values for the provided example data and a little bit more explanation. Some output files will after all differ from the example data output, because of the user interaction.

from joss-reviews.

@sbogutzky, this is much easier for a clueless user like me to get started and know they are running the examples properly. I offer a few more comments below, but please attend in particular to "FIX" in particular.

Actual bug - figure saving dialog on mac doesn't work. It asks you to open an existing file. It should allow to specify a file, right? (should definitely FIX if you have access to a mac. If not, let me know, I'll see if I can figure it out).

I still find some of the visualizations a bit mystifying - for example, the plot after subset-data.R clearly includes some non-activity data, and is mostly a giant black blob (this has been zoomed a bit, resulting in a shift of the data to the right):

Also, I think you mean "knot detection" not "not detection."

In my kicking the tires, get-stride-data.R didn't report when it had trouble finding the file (I entered wrong data). Not sure why that happened... (ideally FIX, but not necessary) Otherwise, this one makes more sense to me in terms of what's plotted. Nice stride, Simon! The final plot again starts as a solid blob.

Minor Type-o FIXes: "Worte" in 'get-jerk-cost-data.R. Also - the README says this doesn't write anything...get-cardiolocomotor-phase-sychronization.R, 'optinal' -> 'optional'. (This plot is again very dense.)

The report is a bit unclear - what is the baseline referring to? Should I just use the running data? It would be nice to have a report that works with the example-data (minor FIX).

from joss-reviews.

Hopefully it's clear from the above what should definitely be fixed - these things are very minor, and I offered to help with the mac issue. Please address those and anything else, and let me know. Should be quick!

from joss-reviews.

@davclark - First thanks for reviewing again!

Unfortunately, by the zoom and printing task I'm also only a user. I use for this task the R package zoom. I also using a mac and have the same bugs. I cannot print from XQuartz and I can also go above the borders of my data.

I meant impossible detection or no detection (fixed)

Worte (fixed); the README said processing/compute-optimal-cut-off-frequencies.R has no output

optinal (fixed)

In the report I load now the example data. The report is for analysis purposes. It is only a template.
Now you have only one subject with one measurement. In real life you have many participant and measurements. Than you can conduct statistical analysis like correlation, regression, ANOVA, effect test, and so on. But that is up to the user. The pipeline only prepares the data.

I guess, I'm ready

from joss-reviews.

As I understand things, this is "good enough" for JOSS. It still lacks automated tests and clear community guidelines, though these don't appear to be necessary for publication per reviewer guidelines.

@arfon let me know if you're ready to proceed on that basis, or if further input is needed!

from joss-reviews.

As I understand things, this is "good enough" for JOSS. It still lacks automated tests and clear community guidelines, though these don't appear to be necessary for publication per reviewer guidelines.

Thanks for the followup @davclark. Our reviewer guidelines are pretty clear on tests but less so on community guidelines.

On tests we say:

Good: An automated test suite hooked up to an external service such as Travis-CI or similar
OK: Documented manual steps that can be followed to check the expected functionality of the software (e.g. a sample input file to assert behaviour)
Bad (not acceptable): No way for you the reviewer to check whether the software works

Looking at https://github.com/sbogutzky/PsychoPhysioPipeline#processing-steps it seems like we're coming in under the 'OK' category here - there is some example data that can be used to verify the functionality.

On community guidelines we say:

There should be clear guidelines for third-parties wishing to:

• Contribute to the software
• Report issues or problems with the software
• Seek support

https://github.com/sbogutzky/PsychoPhysioPipeline#author-and-contribution says:

The authors (Simon Bogutzky) welcome external contributors to freely use and extend this software. If you need some help, please write an issue.

I think this is probably the minimal version of what is required to comply with this requirement.

So yes, I think we're good to move to an accept decision on this paper. @sbogutzky at this point could you make an updated archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

from joss-reviews.

Here ist the batch, the zip is uploaded, but the doi is maybe not registered!?

from joss-reviews.

@sbogutzky thanks for updating the DOI here. @davclark many thanks for the thorough review.

@sbogutzky your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00041 🎉 🚀 💥

from joss-reviews.