JOSS Review Issues - first round

[erik-whiting]

Hi @b-pardi, I'm Erik Whiting, one of the reviewers assigned by the Journal of Open Source Software, which is being manage on this issue here. I am going off the checklist generated by JOSS. I've finished my initial review and found the following issues:

Functionality

Installation

There isn't much in the way of installation instructions. The README has some information, but it's not very thorough and it seems more geared towards an edge case. Personally, I was able to figure out that installing and running BraTaDio meant cloning the repo and running main.py. I don't think you should assume general users will now this, you have to explicitly tell them how to run the software.

I will say, however, that after cloning and installing the requirements, the program ran perfectly.

Functionality

I followed along with the instructions in the README and basically nothing happened. I tried a few things but my most recent run included the following steps:

1. Load the file BraTaDio\sample_generations\qcmi-bsa-after\QSM-I-BSA_1mgpml
2. Select QCM-I
3. Enter the number "10" for both t0 (s) and tf(s)
4. Select "theoretical"
5. Apply default values in plot options
6. Submit file information
7. See success message
8. Click both checkboxes: Raw Data Overtone Selection and Shifted Data Overtone Selection
9. Select "1st frequency" and "1st dissipation" in both
10. Clicked Submit
11. Noticed the following errors in my terminal (I'm not sure at which point they occurred though:
12. Nothing happened after clicking submit. Checked git for generated files, nothing there exception __pycache__ generations (which you should probably include in your .gitignore.
13. Loaded the generated Formatted-QSM-I-BSA_1mgpml file
14. Nothing happened
15. Clicked "Submit file information"
16. Nothing happened
17. Left the same things that are currently "checked", Clicked "Submit"
18. Nothing happened

I may have done something wrong, or it's entirely possible I nothing was supposed to happen. However, it's not clear from your instructions what I should expect to happen. Based on the README, I expected there to be a plot to look at somewhere, but I don't see it.

Also note that after filling everything out, I get the following errors in the console when trying to click any of the buttons in "modeling":

I can't really comment on anything else about the functionality since I cannot seem to make the graphs appear. Please advise on if I am misreading the instructions, if the instructions are unclear, or if there is a bug.

Performance

I think this is N/A since you don't really make any performance claims.

Documentation

Statement of Need

You do cover this in the paper, but the documentation for the repository does not seem to address a statement of need for this software.

Installation Instructions

As I mentioned above, the installation documentation is missing quite a bit.

Automated Tests

These don't seem to be present at all. There are several files with "test" in the filename, but they are all in a directory called deprecated, so it is unclear if they are useful or not. There are also no CI files from what I can tell, so I don't think there is any automated tests.

This checkbox item in the reviewer guide also mentions manual tests for confirming functionality, I don't see any evidence of this anywhere either.

Community Guidelines

There is a bug report file but there are no community guidelines or instructions for developers. I think the standard for these things is to include a CONTRIBUTING file to show people how to report issues and make pull requests, and a .CODE_OF_CONDUCT file for outlining how the community around your software is to behave.

---

Everything else looks pretty good, let me know if you have any questions. I will mark off the different items as they are addressed.

[b-pardi]

@erik-whiting thank you for taking the time out to review the software! The first big thing I want to address is in your step 3, you say you put the same number for t0 and tf. This is an incorrect user since it is supposed to grab data between two different time to points to get the average baseline time. As mentioned in the File information section of the readme, for testing you can use an example range of 0-100 seconds. In practice, this is the last stable few minutes of the device finding an equilibrium before the experiment begins, and is determined during the experiment. This is an input I did not consider, and will act to add error catching to ensure there is a correctly entered time range.

Apologies for leaving out tests, this is my first user interactive project, so I did not know what to consider for tests, I will do some research and add those asap. Same goes for community guidelines, as the sole developer of the project, I was previously just zipping the project and sending updates to my team for use, and the report was made for non-developers to inform me of bugs. But with this in mind I will add more developer oriented reporting mechanisms. I also see your points on the shortcomings of the README and will address those as well.

I should have updates in the next few days, in the meantime please let me know how the rest of the software runs using the baseline time t0 and tf mentioned above.

Thank you again!

[b-pardi]

I believe I have taken care of all the documentation related issues (manual tests, installation instr, etc). I will need some time for the automated tests, but will update when I have that soon.

[erik-whiting]

I don't think purely automated tests are required, the checklist item states that documented manual tests to verify successful installation is good enough!

[b-pardi]

Well I'm glad to hear that! I still plan on adding at least a few to be as thorough as possible, but I'm glad they are not required to continue

[b-pardi]

I believe all things mentioned in this thread have been remedied, please open a new issue if there is anything more. Apologies for not mentioning issues in commits, I forgot that was thing until recently.