Hi @amkrajewski, I am one of the reviewers for your paper: openjournals/joss-reviews#6731
I am copying a portion of the reviewer checklist here, with some notes that I have for improvement. I've checked most of the boxes on the original review, so I will leave those out of this list.
I am also opening a pull request with some suggested changes: #4
I will add some comments on that pull request and link to the corresponding notes in this issue.
Review checklist for @bdice
Functionality
I looked at the "native Python" benchmark first. It appears that the NumPy benchmark is not measuring the same amount of work as the native Python benchmark. For example, the construction of elMatrix
from the input data should be a part of the timing for the NumPy benchmark (and similarly for the nimCSO benchmark). It's okay to compare algorithm runtime that excludes the CSV parsing (such as the cost to create elementalList
in the native Python benchmark) but it is not a fair comparison to pre-process that parsed data by converting into a matrix for NumPy or bit-packed array. Make sure that all benchmarks are measuring as close to the same thing as possible when making relative claims about performance. I understand it is important to amortize that cost of creating a bit-packed array for the input data when computing across many element lists, but it's a bit harder to compare to the native Python solution. I suggest trying to find a middle ground, or at least giving more description of what is being measured.
Documentation
The statement of need describes applications in economics and medicine as well as materials. I understand the materials science application (which is closer to my field of expertise, and is substantiated by the paper) but the economics and medicine use cases need citations.
I'm new to nim
and had some confusion while installing. It seems like it is best practice to use nimble install --depsOnly
rather than listing the dependencies directly in the README. Also, I tried nimble install
and nimble develop
but was unsure how to get the nimCSO
executable onto my PATH. The compile command nim c -r -f -d:release src/nimcso
produces an executable in src/nimCSO.out
but that doesn't seem like the proper "final destination." I am thinking of tools like pip install
that can place entry points or executables directly onto your PATH.
The repository would benefit from a CONTRIBUTING.md. It does not appear that the repository has had many issues or pull requests. It would be good to explicitly instruct users to file issues and pull requests.
I strongly recommend configuring a tool like pre-commit to automatically trim trailing whitespace and format the code. Having enforceable style conventions, even a minimal set, smooths the process of contributing to an open-source project. If you'd like help here, I can explain more and file a PR.
Software paper
I think the paper would benefit from a toy example, explaining what it does. It took me some time to understand precisely what is meant by "selecting components" and phrases like "least preventing". Perhaps a toy example could look something like this?
You run a pizza store and want to simplify the menu. Customers can choose from over 30 toppings. You have sales data about the toppings on every pizza purchase from the last year. Find the best combination of 15 toppings that will satisfy the most orders. How many more orders could be satisfied if you had made a menu with the best 16 toppings? The best 14 toppings?
Obviously this is very contrived, but I think the problem statement and motivation for this work could be clearer, and connect the domain field (materials science / compositionally complex materials) to the broadly-applicable selection algorithms implemented in nimCSO.
Second part of the review begins here
I skimmed the paper on a second read but didn't see any comparisons to other software. How do people solve this problem today? Is it all custom scripts or are there other libraries out there? (e.g. in other programming languages, with different search algorithms, etc.) Especially with the mention of other fields where this is useful (economics, medicine), I think prior art probably exists.
There are a few typos that I have addressed in #4. Generally the writing quality is good.
Some things noted above would be better with a citation (describing examples in economics/medicine, and adding references to other software in this space). Citation style looks fine. One note: the citation for "Genetic algorithms in search, optimization and machine learning" (https://doi.org/10.5860/choice.27-0936) doesn't looks like a useful link. It goes to a login page that has no information about the title. Can you verify this?