search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: G3F: Global, Multidimensional Spectral Regression Analysis #1629

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @dap-biospec (Denis Proshlyakov) Repository: https://github.com/dap-biospec/G3F Version: v1.0a4 Editor: @kyleniemeyer Reviewer: @tcausgrove, @FaustinCarter Archive: 10.5281/zenodo.3377994

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/0654a85582980efe816625e7ff715bcf"><img src="http://joss.theoj.org/papers/0654a85582980efe816625e7ff715bcf/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/0654a85582980efe816625e7ff715bcf/status.svg)](http://joss.theoj.org/papers/0654a85582980efe816625e7ff715bcf)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@tcausgrove & @FaustinCarter, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @kyleniemeyer know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @tcausgrove

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @FaustinCarter

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @tcausgrove, @FaustinCarter it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

kyleniemeyer commented 5 years ago

👋 @dap-biospec @tcausgrove @FaustinCarter the review takes place in here

tcausgrove commented 5 years ago

I'm working through the demo, starting to understand the software. I'm having two issues, which may be related. One, when I open the demo, Igor looks for a "submission_02_works:Demo" folder, which I can't find. (The folder levels in general seem off, extra ::: in front.) Also, for the Process Fitting part of the demo, I can't find the Process_SpecEChem_4Spec_ForOxd_2D function. Am I missing something?

dap-biospec commented 5 years ago

@tcausgrove I updated Demo_G3F.pxp and Performance_Test.pxp after your earlier message. The offending path was deleted. I recommend downloading complete repo as ZIP file to ensure that Igor experiments are current. A characteristic change is checked "Epsilon" checkbox in the main panel. Another check is Misc->"Path Status": QuantAnalysis path should be "[path_to_parent]\G3F-master\G3F\".

The process fitting function Process_SpecEChem_4Spec_ForOxd_2D (and others required for methods 2 and 3) in now in the updated SpecEchem_4Spec_ForOxd_2.ipf procedure file.

dap-biospec commented 5 years ago

@tcausgrove, @FaustinCarter - we updated the demo to exclude unnecessary path (error on load) and to use the correct demo procedure file. Thank you @tcausgrove for bringing it up. Although we tested full download from GitHub before the review, some procedures were still loaded from our network locations instead of relative location in the repo mirror. We identified the cause only after disabling the network on the testing computer. Sorry about that!

Since .gitignore includes *.pxp entry, please make sure that you have the latest demo and testing experiments. Re-downloading repository as ZIP should do the trick.

tcausgrove commented 5 years ago

OK, the demo runs all the way through for me now. Thanks @dap-biospec for the new version. It has taken me a while to figure out how things fit together - there are quite a few moving parts to this software. The learning curve might be a bit of a barrier to adoption.

There are a few things that could probably be addressed more thoroughly in the manual. For example, the items in the Analysis->Global 3D Spectral Regression menu are barely mentioned, but seem very useful.

Some things that I think would be good for future releases:

  1. Expanded manual
  2. A demo using layers
  3. Expanded description of the autocycle feature
  4. Clean up demo - not sure why all of the Pulse... waves are there, how the Reduction data set fits in, or why it uses a 5th order polynomial for the background when all the coefficients are zero
  5. Example of post-processing
  6. Example of using reference data

This is a really extensive system for fitting multi-dimensional data. The software seems to be more or less in finished form, but the documentation could be improved as an ongoing effort. I am ready to recommend acceptance.

dap-biospec commented 5 years ago

Thank you, @tcausgrove. We certainly will continue to develop tutorial moving forward. We are preparing a publication where layers are instrumental for describing isotopic compositions of time-resolved vibrational spectra and we think that this would be a great context to showcase this.

Much of the demo is a simple example taken out of context of our two papers cited herein. We will check for and purge unnecessary data and add a note on the context to the tutorial.

Again, thank you for a speedy review!

cc: @FaustinCarter

dap-biospec commented 5 years ago

@tcausgrove, @FaustinCarter Non-essential data were purged from the demo experiment. Explanations were added to the demo and manual documents to clarify the use of reference data, post-processing functions, and auto-cycling of parameter holds overrides.

dap-biospec commented 5 years ago

@tcausgrove, @FaustinCarter Demo experiment, description, and the manual were updated per requests by @tcausgrove: extra parameters and the post-processing function illustrations.

We believe that we addressed all the issues required by the reviewers. In the absence of additional requests, we hope that the review can be completed soon.

cc: @kyleniemeyer

FaustinCarter commented 5 years ago

I don't have any specific objections to publishing this. It's a cool package, and it has a lot of functionality. However, it is a complicated enough project that I think the documentation could use some expansion; I don't think very many people will use it without some serious hand-holding in the docs. I won't hold the review up over it as long as the authors agree in good faith to take a stab at improving it, but here are my suggestions :

  1. I would like to see some very simple example fits added. The included example is nice, but it is pretty high-level and has too much physical meaning to be a good toy (i.e. the chemistry gets in the way of the mechanics of using the package). My suggestions are:
    1. An arbitrary line in two dimensions with added noise.
    2. An arbitrary line in three dimensions with added noise.
    3. A simple surface in three dimensions with added noise.
    4. Something really simple that makes clear the difference between direct and process fitting. Preferably an example where using the process fitting saves a noticeable amount of time. This would include a comparison of the goodness of fits between the two methods and would help one decide whether the extra computation time might be worth it. For each method, a comparison of the fit values and the extracted uncertainties against the nominal values should be included.
  2. Some discussion on how uncertainties and fit confidence metrics are generated. There is a mention that fit reporting exists, and the JOSS paper says something about Markov Chains, but it isn't at all clear to me from the documentation how I know whether I'm getting reasonable uncertainties from a fit.
  3. Along with the previous point, an explicit statement of what the algorithm(s) behind the fitting is. There are a LOT of ways to do regression (the lmfit package in Python enumerates 21 different algorithms one can use for optimization) and I don't know which one this program is using. For instance are all the residuals for the whole data cube appended together into a line and treated like a 1D problem using Levenberg–Marquardt, etc?

Finally, there are a couple of typos/missing words in the JOSS manuscript (I found one in each of the last two paragraphs, and I may have missed more) so that should get a careful readthrough before it is finalized.

dap-biospec commented 5 years ago

Thank you @FaustinCarter. Point is taken. You are correct - this package transpired from a high-level chemical problems that could not be solved by conventional approaches. We will continue to update documentation with more generic examples and additional scenarios.

kyleniemeyer commented 5 years ago

Thanks for your feedback @FaustinCarter!

@dap-biospec please work on addressing some of those comments, and let us know when you've been able to.

dap-biospec commented 5 years ago

@FaustinCarter thank you for pointing out typos and omissions in the paper. They have been corrected.

The section of the paper describing process fitting has been revised with examples and explanations, keeping a more general scope in mind. I like the idea of simple examples (say, a single Gaussian band with superimposed noise variable over 2D or 3D space), although its careful execution will require more time.

A "general information" section has been added to the beginning of the manual. This section clearly states the algorithm used by G3F, its relationship with IgorPro engine, the source of reported confidence intervals, and the links to the IgorPro manual as the primary source of information.

@FaustinCarter is correct that 3D data are internally linearized and each data point is treated independently. This is the standard approach implemented in IgorPro and, therefore, we do not elaborate on it further. I would be quite interested in exploring other algorithms in the future, including dimension-specific weighting of parameters.

We hope that these changes address the critical concerns and that they would allow to move forward at this time.

cc: @kyleniemeyer

dap-biospec commented 5 years ago

@kyleniemeyer archive has been deposited to Zenodo - DOI is 10.5281/zenodo.3377994 Bibliography has been formatted following an example.

kyleniemeyer commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

kyleniemeyer commented 5 years ago

@FaustinCarter are you satisfied by the changes made?

FaustinCarter commented 5 years ago

I'm happy to move forward with publication.

dap-biospec commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dap-biospec commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dap-biospec commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dap-biospec commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- 10.1073/pnas.0911565107 is OK
- 10.1038/nature14160 is OK
- 10.1021/acs.analchem.9b00859 is OK
- 10.1021/acs.jpcb.9b05866 is OK
- 10.1007/978-1-4419-6766-4_1 is OK
- 10.1007/s11047-019-09736-8 is OK

MISSING DOIs

- None

INVALID DOIs

- None
dap-biospec commented 5 years ago

@whedon set 10.5281/zenodo.3377994 as archive

whedon commented 5 years ago

I'm sorry @dap-biospec, I'm afraid I can't do that. That's something only editors are allowed to do.

dap-biospec commented 5 years ago

@kyleniemeyer - OK, I think I did everything I am permitted to do. I corrected minor style issue in the paper; proof looks good to me.

kyleniemeyer commented 5 years ago

@whedon set 10.5281/zenodo.3377994 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.3377994 is the archive.

kyleniemeyer commented 5 years ago

@dap-biospec I did some copy editing to the paper and made changes in a PR: https://github.com/dap-biospec/G3F/pull/2

There was an issue I couldn't resolve, in the Concept paragraph. There's something off in "obtain completely unknown difference infrared spectra", but I can't figure out what it should be. Maybe "obtain completely unknown differences in infrared spectra"?

tcausgrove commented 5 years ago

I think they are getting at something like "analyze a set of measurements to determine previously unknown difference spectra". It seems the object is to separate chemical species based on perhaps incomplete oxidation/reduction. JMO.

dap-biospec commented 5 years ago

@kyleniemeyer - merge is done. @tcausgrove is correct - I deleted the word "infrared" as it is an irrelevant detail in a broader context.

kyleniemeyer commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

kyleniemeyer commented 5 years ago

OK, all looks good here now!

kyleniemeyer commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/933

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/933, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
kyleniemeyer commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago 
Doing it live! Attempting automated processing of paper acceptance...