[JOSS review] Initial comments

[ml-evs]

Hi @sgbaird, just going to use this issue to track anything I spot on my first playaround with this:

• [x] Incorrect link for the metrics description in the README
• [x] Missing release v0.6.1 in GitHub (but present on PyPI)
• [x] CHANGELOG.md has never been filled in (I guess it can just be removed)
• [x] LICENSE.txt should probably use your full name rather than GitHub handle (though guess that's just preference)
• [x] Unclear which dependencies are the "real" ones, environment.yml has much of it commented out (and has no pinned versions), requirements.txt is just a single line for a package not mentioned anywhere else, so I guess setup.cfg is the real one? I can't find any pinned dependencies anywhere which will hinder reproducibility going forward. This also seems to go against the Dependency Management section of the README
• [x] The folders configs, data, models, raw, and references don't seem to contain anything useful
• [x] Provided Dockerfile does not successfully build on my machine, looks like numba fails to install due to missing gcc, if this is not a supported mechanism then I suggest removing the Dockerfile (or at least massively simplifying it, just using the base Python image and running pip install matbench-genmetrics is probably helpful enough)
• [x] The paper refers to random tex files in this repository; these links are not persistent and are hard to follow without compiling the documents myself. If these are important they should be added to the package documentation, e.g., https://github.com/sparks-baird/matbench-genmetrics/blob/main/reports/matbench-genmetrics.tex
• [ ] It is hard to know if the metrics are actually doing the right thing given that the only examples provided run on random structures in a much reduced set. To assess the "performance/functionality" of the library (a JOSS criteria) I think I would need to see it run on some more useful test cases.
• [x] Very limited documentation on utils methods, which seem to implement a lot of the package functionality. Are these meant to be "private"/non-user-facing methods? (e.g., https://matbench-genmetrics.readthedocs.io/en/latest/api/matbench_genmetrics.core.utils.html#module-matbench_genmetrics.core.utils.featurize)

[sgbaird]

Hi Matt, I addressed all points (can you verify the Dockerfile works now?) except for the "slow march of time" performance assessment which @hasan-sayeed will be taking point on. Can you let me know if these are adequate?

[ml-evs]

Hi @sgbaird, thanks for the work implementing these changes. I can confirm the docker build works at my end, though when I run the example with the built image (after tweaking the import as per https://github.com/sparks-baird/matbench-genmetrics/issues/81, I get the infamous yaml error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.10/site-packages/matbench_genmetrics/core/metrics.py", line 16, in <module>
    from matbench_genmetrics.core.utils.featurize import (
  File "/usr/local/lib/python3.10/site-packages/matbench_genmetrics/core/utils/featurize.py", line 13, in <module>
    from matbench_genmetrics.core.utils.match import get_tqdm
  File "/usr/local/lib/python3.10/site-packages/matbench_genmetrics/core/utils/match.py", line 73, in <module>
    CrystalNNFP = CrystalNNFingerprint.from_preset("ops")
  File "/usr/local/lib/python3.10/site-packages/matminer/featurizers/site/fingerprint.py", line 388, in from_preset
    cn_target_motif_op = load_cn_target_motif_op()
  File "/usr/local/lib/python3.10/site-packages/matminer/featurizers/site/fingerprint.py", line 887, in load_cn_target_motif_op
    return yaml.safe_load(f)
  File "/usr/local/lib/python3.10/site-packages/ruamel/yaml/main.py", line 1105, in safe_load
    error_deprecation('safe_load', 'load', arg="typ='safe', pure=True")
  File "/usr/local/lib/python3.10/site-packages/ruamel/yaml/main.py", line 1039, in error_deprecation
    raise AttributeError(s, name=None)
AttributeError:
"safe_load()" has been removed, use

  yaml = YAML(typ='safe', pure=True)
  yaml.load(...)

instead of file "/usr/local/lib/python3.10/site-packages/matminer/featurizers/site/fingerprint.py", line 887

        return yaml.safe_load(f)

and it seems like an incompatible yaml version is installed (relative to the upper pin of 0.18 in the setup.cfg):

root ➜ / $ pip freeze | grep yaml
ruamel.yaml==0.18.5
ruamel.yaml.clib==0.2.8

I guess this is because the PyPI version does not have that pin yet? I would probably suggest that this docker build isn't that useful if it just mimics a user running pip install themselves (all that is being held constant is the Python version in that case), but that's not the end of the world. Feel free to close this if you are happy -- we can track the more general issue (better motivating examples) in the main JOSS thread.