Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[REVIEW]: fathon: A Python package for a fast computation of detrendend fluctuation analysis and related algorithms #1828
Comments
This comment has been minimized.
This comment has been minimized.
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ali-ramadhan, @ashwinvis it looks like you're currently assigned to review this paper 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
I like the idea of having a simple and intuitive open-source Python package offering numerous DFA algorithms and the examples look great, but I think fathon needs some work to make it more accessible to potential users. My major comments are:
Maybe @mbobra can confirm whether these three points are necessary for publication in JOSS. I do think they would improve fathon and make the package much more accessible to potential users. A more detailed review follows below. I plan to come back and finish this review once I can install fathon. FunctionalityInstallation
I wonder if providing a Python package installable via GSL seems to be a pre-requisite which is available through most Linux package managers. Installing fathon on Windows might prove difficult and it looks like you might need to install all of Cygwin just to get GSL to install fathon. I might mention this as most Windows users have no idea what Cygwin is. I was not able to install fathon myself. I opened an issue about this: stfbnc/fathon#1 I believe the installation instructions are unfriendly. fathon seems much harder to install for me than the vast majority of Python packages, many of them which depend on C libraries too. Besides having to manually install or compile GSL, you also need to know where include and library files are placed on your operating system, and you need to know the "standard location of Python packages" for your particular OS and Python environment combination. I think this is a huge barrier to usability and most Python users won't be able to install fathon. I opened an issue to ask whether providing a FunctionalityI was not able to install fathon but when I do I will try to confirm this. I'm not sure if whether four tests on Gaussian white noise are sufficient to claim that fathon fully works. That seems to be the easiest test case. I would like to explore testing other cases. PerformanceNo performance claims I could find except for "It is mostly written in Cython and C in order to speed up computations" which seems reasonable. DocumentationA statement of needSome motivation is provided in the JOSS paper and documentation. Installation instructionsThere is a clearly-stated list of depdencies (GSL) although Cython might be missing (see stfbnc/fathon#1). I strongly believe that fathon will benefit from having installation handled via an automated package managemnent solution such as ExamplesThe Functionality documentationI could not find any documentation for the API. I also think having the documentation in PDF form is detrimental to being simple and user friendly as it becomes difficult to link to them and it seems that nobody can contribute to improving the docs as the LaTeX document is not part of the repository. I believe with documentation packages like Sphinx, documenting the API becomes easy, and it's trivial to deploy the docs to a website like ReadTheDocs. Looking through the code, there are really good docstrings so it's a shame that they're currently hidden from the user! Automated testsThere are no automated tests, and no continuous integration. There is a Jupyter notebook and four Python scripts provided where the burden is on the user to run the tests. I think it would be easy to convert the four scripts to automated tests that run on a CI platform like Travis CI. I haven't set up Travis with Python but it was painless with other languages, so I assume it should be easy to set up. Community guidelinesI could not find any community guidelines or a contributor's guide. There should be "clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support". This can all be included in the README I believe. I find it kind of weird that the README states "For bugs or improvements, write to fathon.package@gmail.com" when the package is on GitHub. I think the README should strongly encourage the submission of GitHub issues so that bugs and improvements can be visible to all users and the discussion can be more transparent. This also allows other people to contribute as it's not dependent on one person checking an email inbox and having the time to reply. Keeping track of issues on GitHub also leaves behind a rich history of package development that allows other contributors to take over if the original package developer no longer has the time to develop the package. Software paperSummaryI think the first paragraph is not accessible to a "diverse, non-specialist audience". DFA itself isn't actually described. Terms such as "fluctuation function", "Hurst exponent", and "anti-persistent" would not be understood by non-specialists I think. I also think the mass citation of ~10 different works without context on the second sentence is not helpful to readers, especially non-specialists. I would suggest picking 2-4 works and placing them in context so that a non-specialist reader can understand how DFA was used in that particular work and why it was useful. Statements such as "found various applications in other fields, like geophysics or economy" seem vague and not very helpful. It can be argued that the entire field of mathematics and statistics has found found various applications in other fields, like geophysics or economy. The rest of the paper looks good to me. State of the fieldA quick Google search yielded three results for open-source Python packages that provide DFA algorithms:
It doesn't have to enter the README or JOSS paper but I'm curious how fathon is different. Do the others not provide the four algorithms provided by fathon? Are these packages worth mentioning? A MATLAB package is mentioned in the JOSS paper which I assume is more comprehensive than these Python packages? Quality of writingIn the first paragraph, I think you mean "economics" instead of "economy". Otherwise I think it's fine. ReferencesThese seem fine to me, but as mentioned above I think there may be too many. I think "co_2" and "mauna loa" should be captialized to "CO_2" and "Mauna Loa" for the Varotsos et al. (2007) reference. Also missing some captialization in the Janosi et al. (1999) reference. Should be "Also I think Statistical analysis of 5 s index data of the Budapest Stock Exchange". "matlab" should be "Matlab" in the Ihlen (2012) reference. |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@mbobra Working on it :) |
This comment has been minimized.
This comment has been minimized.
Thanks for your detailed review, @ali-ramadhan. Here are my replies:
Does this help, @ali-ramadhan? |
This comment has been minimized.
This comment has been minimized.
Thank you @mbobra, that's helpful! Sorry it's my first review but I should have familiarized myself with the review guidelines more closely.
I would be happy with clear installation instructions that can be followed by people without having to spend too much time figuring out where GSL include/library files are located and where Python packages are usually installed.
I think fathon satisfies the first three and does technically satisfy the fourth as functions in the source code are documented with good docstrings. I feel like it doesn't count as API documentation though unless the docstrings are compiled into documentation, but maybe having docstrings in the source code meets the JOSS requirements.
The manual steps in the README sound good then. |
This comment has been minimized.
This comment has been minimized.
@ali-ramadhan Sounds good! @stfbnc Let me know if you have any questions or need any help resolving these issues. |
This comment has been minimized.
This comment has been minimized.
@ali-ramadhan Thanks for the comments, I'm now working on resolving the issues and on the documentation. @mbobra Can I modify the paper as ali-ramadhan suggested, or should I wait for the second reviewer's comments? |
This comment has been minimized.
This comment has been minimized.
@stfbnc Yes, feel free to modify the paper right away! Every time you modify, you can make a comment in this thread with the command |
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@ali-ramadhan I have corrected the paper as you suggested, there is now a theoretical introduction on the algorithms covered by fathon. For the State of the field section:
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@ali-ramadhan @ashwinvis Sorry for the late reply. I have updated the repository. Requirements are now specified, along with the new installation guidelines. I have also added:
|
This comment has been minimized.
This comment has been minimized.
Code coverage is by now 0%, but only because the package is written in cython and not python. I'm still trying to figure out how to make |
This comment has been minimized.
This comment has been minimized.
@mbobra Thanks for the nudge. I was coordinating with the author to improve the installation and packaging situation stfbnc/fathon#1 (comment) so that we have a version that can be reviewed. @stfbnc Code coverage is not necessary for the paper, but if you are willing to go that extra mile, it can be fixed by following this document. You need to enable |
This comment has been minimized.
This comment has been minimized.
@stfbnc Let me know your timeline/plans for this paper. You should feel absolutely free to take as much time as you need, so there's no rush or anything. I only want to know the timeline because if it is long (>1 month or so) we can add a "paused" label to this review to indicate to the Editors in Chief that they don't need to watch over this thread in the interim. |
This comment has been minimized.
This comment has been minimized.
/ooo December 16 until January 6 |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@whedon check references |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@mbobra DOIs added! |
This comment has been minimized.
This comment has been minimized.
@whedon set 10.5281/zenodo.3601637 as archive |
This comment has been minimized.
This comment has been minimized.
OK. 10.5281/zenodo.3601637 is the archive. |
This comment has been minimized.
This comment has been minimized.
@whedon set v0.1.2 as version |
This comment has been minimized.
This comment has been minimized.
OK. v0.1.2 is the version. |
This comment has been minimized.
This comment has been minimized.
@openjournals/joss-eics This paper is accepted and ready for final processing! Nice work, @stfbnc |
This comment has been minimized.
This comment has been minimized.
Thanks @mbobra - just as a minor correction, let's call it "ready-to-accept" until the final processing is done :) |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Check final proof If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1209, then you can now move forward with accepting the submission by compiling again with the flag
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@danielskatz Thanks for the corrections, I've merged your pull request. |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Check final proof If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1210, then you can now move forward with accepting the submission by compiling again with the flag
|
This comment has been minimized.
This comment has been minimized.
@whedon accept deposit=true |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Here's what you must now do:
Any issues? notify your editorial technical team... |
This comment has been minimized.
This comment has been minimized.
Thanks to @ali-ramadhan & @ashwinvis for reviewing, and @mbobra for editing! |
This comment has been minimized.
This comment has been minimized.
If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: 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:
|
whedon commentedOct 22, 2019
•
edited
Submitting author: @stfbnc (Stefano Bianchi)
Repository: https://github.com/stfbnc/fathon.git
Version: v0.1.2
Editor: @mbobra
Reviewer: @ali-ramadhan, @ashwinvis
Archive: 10.5281/zenodo.3601637
Status
Status badge code:
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
@ali-ramadhan & @ashwinvis, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @mbobra know.
Review checklist for @ali-ramadhan
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @ashwinvis
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper