[REVIEW]: Comet Time Series (CometTS) Visualizer

whedon commented 5 years ago

Submitting author: @jshermeyer (Jacob Shermeyer) Repository: https://github.com/CosmiQ/CometTS Version: <v1.2.0> Editor: @Kevin-Mattheus-Moerman Reviewers: @zhampel, @rmsare, @jjmcnelis Archive: 10.5281/zenodo.3418091

Status

Status badge code:

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

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) 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

@zhampel, and @rmsare please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

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

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @Kevin-Mattheus-Moerman know.

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

Review checklist for @zhampel

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: <v1.2.0>
[x] Authorship: Has the submitting author (@jshermeyer) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

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

Review checklist for @rmsare

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: <v1.2.0>
[x] Authorship: Has the submitting author (@jshermeyer) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

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

Review checklist for @jjmcnelis

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: <v1.2.0>
[x] Authorship: Has the submitting author (@jshermeyer) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

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

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @rhiever, it looks like you're currently assigned as the reviewer for 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:

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

watching

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

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:

Kevin-Mattheus-Moerman commented 5 years ago

@rhiever, @zhampel this is where the review process takes place. There is information and check boxes at the top of this issue to guide you through the review process. Let me know if you have any questions.

zhampel commented 5 years ago

@Kevin-Mattheus-Moerman I must disclose an affiliation I have with the author @jshermeyer of this submission. We are employees of the same parent organization 'IQT', though we have affiliations with separate independent laboratories within IQT. I leave it to you to decide whether I proceed as a reviewer.

Kevin-Mattheus-Moerman commented 5 years ago

@zhampel thanks for disclosing that. Can you provide a link to that organization IQT?

zhampel commented 5 years ago

@Kevin-Mattheus-Moerman Sure thing: https://www.iqt.org

Kevin-Mattheus-Moerman commented 5 years ago

@whedon add @dmittman as reviewer

whedon commented 5 years ago

OK, @dmittman is now a reviewer

Kevin-Mattheus-Moerman commented 5 years ago

@zhampel thanks for mentioning your shared affiliation. I do not feel this is serious enough to exclude you from the review process. I'm confident that, now that we have three reviewers, and since review takes place in the open, we'll have a balanced review process. Thanks.

Kevin-Mattheus-Moerman commented 5 years ago

@rhiever, @zhampel, @dmittman thanks again for acting as reviewers. You all have a set of tick boxes at the top of this issue which will guide you through the review process (which you can tick if you've accepted this invitation). Please let me know if you have any questions. :rocket: :robot:

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer can you work on adding community/contributing guidelines (e.g. CONTRIBUTING.md)? I also recommend adding a code of conduct document (e.g. COC.md, you can find a good standard template here: https://www.contributor-covenant.org/). I recommend you link to the contributing guidelines in the readme and to the code of conduct in the contributing guidelines.

Examples: https://github.com/atom/atom/blob/master/CONTRIBUTING.md https://github.com/rails/rails/blob/master/CONTRIBUTING.md

zhampel commented 5 years ago

@jshermeyer Here are my initial comments on the paper.

Paper

General comments:

I think it would be better to intro something like ‘user defined polygon identifying a region of interest (ROI)’ and use ROI for the remainder of the paper.
Q: Any limits to ‘user defined area of interest’?
Figures: please provide descriptive captions to the three figures.

Specific correction suggestions:

I feel like the paper title could be smooth like this: 'Comet Time Series Visualizer: CometTS'
that we call -> called
(CometTS) -> (CometTS)
a times series of satellite imagery -> time series satellite imagery.
-> The tool aims to enable research into population estimation, land use detection, or natural disaster monitoring using a variety of data types.
series of satellite imagery -> series of satellite images
variation), -> variation)
visualizes -> provides a visualization of
from user drawn polygons. -> from a user defined region of interest.
define GIS as geographic information systems
can only be used to analyze individual -> are limited to analysis of individual
CometTS then produces -> CometTS produces
within the polygon -> region of interest
of most relevance -> most relevant
standard statistics -> standard statistical measures / standard test statistics

jshermeyer commented 5 years ago

@Kevin-Mattheus-Moerman Added a CONTRIBUTING.MD. Code of conduct seems overkill, if I need to post a code of conduct calling for basic civility in my little codebase I think the world is doomed.

@zhampel Thanks for the notes. Will make some edits when another review comes in.

I think it would be better to intro something like ‘user defined polygon identifying a region of interest (ROI)’ and use ROI for the remainder of the paper.

Agreed, will make this change then when other reviews come back.

Q: Any limits to ‘user defined area of interest’?

Just computational strength, the larger the polygon the more slowly the processing. I believe CometTS will not like polygons that are larger than the imagery you provide, but it can tolerate and ignore nodata/ blank space/ masked areas.

Figures: please provide descriptive captions to the three figures. Good idea.

Kevin-Mattheus-Moerman commented 5 years ago

@zhampel, @dmittman, can you provide an update on where we are in the review process? @rhiever when can we expect your contribution? Thanks!

dmittman commented 5 years ago

Apologies for the delay @Kevin-Mattheus-Moerman. I've reviewed the reviewer guidelines, checked the conflict of interest statement and took a look through the submitted PDF and repo. I find the material just outside my area of expertise, being more about satellite imagery analysis with a time-series component than pure time-series visualization. Perhaps @lewismc (https://github.com/lewismc) might be a more appropriate reviewer than I.

Kevin-Mattheus-Moerman commented 5 years ago

@dmittman Thanks for your reply. I understand. Thanks for taking the time to check out this project, and for recommending another reviewer. @lewismc would you be interested in reviewing this work for JOSS?

lewismc commented 5 years ago

@Kevin-Mattheus-Moerman yes I would. Please give me a deadline for reviewing. I've just returned from some vacation.

Kevin-Mattheus-Moerman commented 5 years ago

@whedon add @lewismc as reviewer

whedon commented 5 years ago

OK, @lewismc is now a reviewer

Kevin-Mattheus-Moerman commented 5 years ago

@whedon remove @dmittman as reviewer

whedon commented 5 years ago

OK, @dmittman is no longer a reviewer

Kevin-Mattheus-Moerman commented 5 years ago

Great @lewismc thanks for agreeing to help. I've added you as a reviewer. It would be great to get this review done in about 2 weeks. Does that work?

zhampel commented 5 years ago

@Kevin-Mattheus-Moerman Sorry for the delay. Yes, I've nearly finished through the code, and will be posting my review by early next week.

lewismc commented 5 years ago

@Kevin-Mattheus-Moerman please point me at what it is you want me to review. Thank you

Kevin-Mattheus-Moerman commented 5 years ago

@lewismc the review focuses on the software and the short paper. The instructions and tick boxes (see the top of this issue) will guide you through the process. Let me know if you still have questions. You can also consult our review criteria here: https://joss.readthedocs.io/en/latest/review_criteria.html.

zhampel commented 5 years ago

@jshermeyer here are my comments regarding the structure of CometTS's code. I found your instructions easy to follow with the tools you have built. Your code does work as advertised, though I do think there are a changes that should be made to the formatting of the code, including commenting and other coding standards that should be upheld. Furthermore, I think your work could be made much stronger as an extensible project, by migrating the functionality of your python notebooks to a more API-like structure. I would also like to point out that a significant hurdle to properly reviewing the project is the lack of easy access to example datasets (you do point this out), which is beyond the scope of you as the author. To this end, the open source community encourages steps like CometTS to making these tools openly available indeed.

Code Review

General

MacOS fails to pip install due to gdal. Needs brew install gdal prior to pip install -r Requirements.txt. Works on my MacOS Mojave laptop.
DownThemAll add-on link points to a non-existent page.
Ensuring a proper directory structure is clear for the example you gave, but is that the case with other datasets? Not being a geospatial data expert, is there a resource that provides guidance on proper structuring, or perhaps a universal industry standard?
Had difficulty with GDAL install (who doesn’t?). Perhaps best to include gdal and subsequent deps on gdal at the end of the requirements list, so that all other packages install first. This may assist in debugging install.
Also recommend including link to building GDAL with python-bindings in Installation section. Had success using following: http://trac.osgeo.org/gdal/wiki/BuildingOnUnix
Please include a python style .gitignore file that excludes the CometTS python notebooks.

Code Specifics

Recommend reformatting for python3 compatibility
Recommended to import functions explicitly, instead of * for ease of tracking
Header description for each fn
Inputs and outputs description
All input argument declarations should be initialized to None, ‘’, etc
Run flake8 to format code properly, will make it easier to read too ;)
Multi_studyAreas_FullStats.csv is in same dir as notebooks. Plot_Results.ipynb looks elsewhere for it. Consider moving it into the example data dir. Yet, when trying to use this example csv data, I get an error that there is no key count. Did work on my generated csv data though.
For plotting, should have more scalable y-axis, as I chose a rather small city (Santa Fe, NM) to test
Difficult to debug errors, esp wrt formatting data issues
What are the temp*.vrt files? Are they part of the project?
Need a set of tests for function verifications

Kevin-Mattheus-Moerman commented 5 years ago

Best wishes for 2019 everybody.

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer can you reply to @zhampel 's comments?

Kevin-Mattheus-Moerman commented 5 years ago

@rhiever, @lewismc can you provide an update on the review process? Let me know if you need help. Also please let us know in case you are no longer able to assist in this review. Thanks.

jshermeyer commented 5 years ago

@Kevin-Mattheus-Moerman Thanks for staying on this. I'll do a full reply to all comments once all parties are done reviewing.

Kevin-Mattheus-Moerman commented 5 years ago

@rhiever has indicated he is no longer able to review this work

Kevin-Mattheus-Moerman commented 5 years ago

@whedon remove @rhiever as reviewer

whedon commented 5 years ago

OK, @rhiever is no longer a reviewer

Kevin-Mattheus-Moerman commented 5 years ago

@lewismc please let me know if you are able to proceed with this review. We could really use your help. Thanks.

Kevin-Mattheus-Moerman commented 5 years ago

@lewismc can you give an update on this review? Are you still able to help us? Thanks!

Kevin-Mattheus-Moerman commented 5 years ago

@lewismc Please can you inform us if you are still able to do this review. Thanks

Kevin-Mattheus-Moerman commented 5 years ago

I've e-mailed @lewismc to check if he is still able to review. Apologies for the delay encountered so far.

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer although I will continue to try to contact @lewismc I suggest you start addressing comments by @zhampel to avoid further delays.

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer in particular can you comment on whether you agree with @zhampel to migrate "... the functionality of your python notebooks to a more API-like structure..."?

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer :point_up:

labarba commented 5 years ago

👋 It looks like we're waiting here for the author, @jshermeyer, to come back with responses to how they are addressing the reviewer comments.

It also looks like the second reviewer, @lewismc, has gone MIA (unresponsive to multiple mentions here, plus emails from the editor). I suggest we proceed to a recommendation from the first reviewer and make the acceptance decision at that point.

jshermeyer commented 5 years ago

@jshermeyer here are my comments regarding the structure of CometTS's code. I found your instructions easy to follow with the tools you have built. Your code does work as advertised, though I do think there are a changes that should be made to the formatting of the code, including commenting and other coding standards that should be upheld. Furthermore, I think your work could be made much stronger as an extensible project, by migrating the functionality of your python notebooks to a more API-like structure. I would also like to point out that a significant hurdle to properly reviewing the project is the lack of easy access to example datasets (you do point this out), which is beyond the scope of you as the author. To this end, the open source community encourages steps like CometTS to making these tools openly available indeed.

Thanks to Zig @zhampel for his helpful and thoughtful comments. Also thanks to the editors @Kevin-Mattheus-Moerman @labarba for pushing this on, I apologize for being an absentee on this. I've gradually added onto Comet to address all of his concerns.

A brief overview: -Code totally reformatted to meet pep8 standards and reorganized for simplicity -Now has a command line API for those who want to ignore the notebooks (would still recommend notebooks for easy plotting/viz). -Installable via pip/conda -Dockerized -Pytests included for sensitive functions -Added support for autoregressive integrated moving average modeling for future trend forecasting -Includes an example test dataset of NPP VIIRS monthly composites over San Juan, PR.

Specifics:

MacOS fails to pip install due to gdal. Needs brew install gdal prior to pip install -r Requirements.txt. Works on my MacOS Mojave laptop.

Updated to have gdal install at the end. Also include support with docker and a conda environment for mac users. Would not recommend brew installing gdal, things get ugly fast when using brew/pip/conda.

DownThemAll add-on link points to a non-existent page.

Still works for old firefox, unfortunately isn't supported any more. I'll leave it up to an end user to get their own data. I now include sample VIIRS imagery for play.

Ensuring a proper directory structure is clear for the example you gave, but is that the case with other datasets? Not being a geospatial data expert, is there a resource that provides guidance on proper structuring, or perhaps a universal industry standard?

This is a standard for a few time-series methods, i.e. CCDC/YATSM- https://github.com/ceholden/yatsm

Had difficulty with GDAL install (who doesn’t?). Perhaps best to include gdal and subsequent deps on gdal at the end of the requirements list, so that all other packages install first. This may assist in debugging install.

Done.

Also recommend including link to building GDAL with python-bindings in Installation section. Had success using following: http://trac.osgeo.org/gdal/wiki/BuildingOnUnix

Multiple choices for installing GDAL now available.

Please include a python style .gitignore file that excludes the CometTS python notebooks.

Notebooks are pretty core for this project, but project is now pip/conda installable with an option for CLI only so this seems redundant.

Code Specifics

Recommend reformatting for python3 compatibility

Done.

Recommended to import functions explicitly, instead of * for ease of tracking

Done.

Header description for each fn/ Inputs and outputs description

Now have comments that describe inputs/outputs + github readme

All input argument declarations should be initialized to None, ‘’, etc

Some of these I like to keep to give a user what an example input would look like.

Run flake8 to format code properly, will make it easier to read too ;)

Done

Multi_studyAreas_FullStats.csv is in same dir as notebooks. Plot_Results.ipynb looks elsewhere for it. Consider moving it into the example data dir. Yet, when trying to use this example csv data, I get an error that there is no key count. Did work on my generated csv data though.

Code structure has been totally reorganized

For plotting, should have more scalable y-axis, as I chose a rather small city (Santa Fe, NM) to test

I've included a few helper notebooks for plotting only. A user will have to have a bit of background with matplotlib for tweaking some of the plotting features.

Difficult to debug errors, esp wrt formatting data issues

Not really sure how to make this easier, probably will require more iterations. Have added pytests to hopefully alleviate some stress.

What are the temp*.vrt files? Are they part of the project?

Temp outputs from some functions, can be ignored. I likely should clean these up but could be helpful for debug.

Need a set of tests for function verifications

Included for a good chunk of the code that could be sensitive. Also include test data.

Kevin-Mattheus-Moerman commented 5 years ago

@labarba thanks for helping here. In relation to:

It also looks like the second reviewer, @lewismc, has gone MIA (unresponsive to multiple mentions here, plus emails from the editor). I suggest we proceed to a recommendation from the first reviewer and make the acceptance decision at that point.

Agreed, however, although I do not feel there is evidence for a clear conflict of interest here (in fact the reviewer has been very thorough and detailed requirements), the reviewer @zhampel did point out to me that "...the author, Jake Shermeyer, is a member of a sister lab of my organization.". Hence I would prefer to find a replacement reviewer.

Kevin-Mattheus-Moerman commented 5 years ago

@chrismattmann @dmittman I believe you pointed out you might be able to review this work over at https://github.com/openjournals/joss-reviews/issues/866. Would greatly appreciate if you would be able to help (save the day! :rocket: ) with this review? The review has already started but we now need at least one additional/replacement reviewer.

Kevin-Mattheus-Moerman commented 5 years ago

@jshermeyer can you check if you need to update your paper to reflect all the changes you've made? Please run @whedon generate pdf here to regenerate your paper here.

jshermeyer commented 5 years ago

No changes required.

Kevin-Mattheus-Moerman commented 5 years ago

@chrismattmann @dmittman :wave: do you think you can help review this work? @danielskatz do you know these potential reviewers personally? We've had a reviewer drop out so we need a replacement. Thanks.

danielskatz commented 5 years ago

I sent an email in case they don't see the github notification, but it's also just 4:30 am in California ...

dmittman commented 5 years ago

I find the material just outside my area of expertise, being more about satellite imagery analysis with a time-series component than pure time-series visualization.

openjournals / joss-reviews