Skip to content
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

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

Closed
36 tasks done
whedon opened this issue Aug 6, 2019 · 55 comments
Closed
36 tasks done

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

whedon opened this issue Aug 6, 2019 · 55 comments
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Aug 6, 2019

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

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.0a4)?
  • Authorship: Has the submitting author (@dap-biospec) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @FaustinCarter

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.0a4)?
  • Authorship: Has the submitting author (@dap-biospec) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon
Copy link
Author

whedon commented Aug 6, 2019

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 🎉.

⭐ Important ⭐

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
Copy link
Author

whedon commented Aug 6, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 6, 2019

@kyleniemeyer
Copy link

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

@tcausgrove
Copy link

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
Copy link

@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
Copy link

@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
Copy link

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
Copy link

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
Copy link

@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
Copy link

@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
Copy link

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
Copy link

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
Copy link

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
Copy link

@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
Copy link

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

@kyleniemeyer
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Aug 26, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 26, 2019

@kyleniemeyer
Copy link

@FaustinCarter are you satisfied by the changes made?

@FaustinCarter
Copy link

I'm happy to move forward with publication.

@dap-biospec
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 27, 2019

@dap-biospec
Copy link

@whedon generate pdf

@dap-biospec
Copy link

@whedon check references

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Attempting to check references...

@whedon
Copy link
Author

whedon commented Aug 27, 2019


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
Copy link

@whedon set 10.5281/zenodo.3377994 as archive

@whedon
Copy link
Author

whedon commented Aug 27, 2019

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

@dap-biospec
Copy link

@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
Copy link

@whedon set 10.5281/zenodo.3377994 as archive

@whedon
Copy link
Author

whedon commented Aug 27, 2019

OK. 10.5281/zenodo.3377994 is the archive.

@kyleniemeyer
Copy link

@dap-biospec I did some copy editing to the paper and made changes in a PR: dap-biospec/G3F#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
Copy link

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
Copy link

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

@kyleniemeyer
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 27, 2019

@kyleniemeyer
Copy link

OK, all looks good here now!

@kyleniemeyer
Copy link

@whedon accept

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Attempting dry run of processing paper acceptance...

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Check final proof 👉 openjournals/joss-papers#933

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#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
Copy link

@whedon accept deposit=true

@whedon
Copy link
Author

whedon commented Aug 27, 2019

Doing it live! Attempting automated processing of paper acceptance...

@whedon
Copy link
Author

whedon commented Aug 27, 2019

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon
Copy link
Author

whedon commented Aug 27, 2019

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.01629 joss-papers#934
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01629
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@kyleniemeyer
Copy link

Congrats @dap-biospec on your article's publication in JOSS! Many thanks to @tcausgrove and @FaustinCarter for reviewing.

@whedon
Copy link
Author

whedon commented Aug 27, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.01629/status.svg)](https://doi.org/10.21105/joss.01629)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01629">
  <img src="https://joss.theoj.org/papers/10.21105/joss.01629/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.01629/status.svg
   :target: https://doi.org/10.21105/joss.01629

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@dap-biospec
Copy link

Thank you @tcausgrove, @FaustinCarter, and @kyleniemeyer for guidance, support, and patience! It is much appreciated!

@whedon whedon added published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. labels Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

5 participants