[joss review] review from @enricgrau

[enricgrau]

Regarding this JOSS review

Overall, the library shows a clear motivation and solution, along with comprehensive documentation, examples, and tutorials. I’d support for publication in JOSS after addressing my comments and concerns herein.

Manuscript
The paper is well written and structured. It presents a clear motivation and solution with the library. The authors do a good job of describing the library, its structure, and its use. Here are some observations to improve the current state of the manuscript:

1. The first sentence states that “thermoelectric materials could be an important renewable energy source to help slow the climate crisis”. Why? How? Examples of this? This feels like a random phrase without further context. However, I do not think it is necessary to include it in the manuscript as the identified problem is motivation and justification enough for the library. In other words, I suggest removing this line or developing a broader context on why and how these materials can help with the climate crisis.
2. The authors claim the library has been used in the literature, however, explicit mention of the library is not present in the cited articles. There is mention of the library ThermoPlotter in Kavenagh (2021), Herring Rodriguez (2023), Brlec (2022), and Spooner (2021) which looks to be a previous version of ThermParser. Clarifying this in the manuscript would increase confidence in this claim. However, I did not find any mention in neither Spooner (2020) or Einhorn (2020). Even though I’m not specifically questioning the use of the library on those specific articles, I don’t think it corresponds to include them due to the lack of any explicit mention of it. Please correct me if there is any kind of mention of the library in said articles that I did not catch.
3. I believe there is a typo at L.16: BoltzTrap(2), other mentions of the library are L.50: BoltzTraP, L.155: BoltzTraP…
4. Phrasing could be better in L.95-97. Is the “;” a typo? Maybe a bullet point structure could improve the understanding of future directions of the library (if there are several)
5. For Author Contribution I recommend using CRediT for a clearer and fair characterization of the authors’ contribution.

Documentation
Gallery

• The Tutorials links are broken
• Clicking in figures links to a broken URL
• The reference is not in the correct format. It displays the bibtex entry rather than “1. Maradudin, ….”

Tutorials
It is great that you show both CLI and Python options and the thorough explanation of what each stage is doing and which lines they correspond to. Good job! However, I did have trouble running them. Here are some comments:

• There is no context on the data. You should include what kind of data the tutorial is working with to better understand its application (e.g. Where does the data come from? How was it measured?)
• Links in the titles are broken (in https://smtg-bham.github.io/ThermoParser/tutorials.html), but in the menu they work.
• Tutorial-02: no file kappa-m404021.hdf5 is incldued
• Tutorial-03: with CLI I get raise ValueError(f"{v!r} is not a valid value for {k}") ValueError: "'#f0901f'" is not a valid value for color. Python works.
• Tutorial-04: <sub>3</sub> appears as text
• Tutorial-04: file kappa-m363636.hddf5 not included
• Tutorial-05: file kappa-m363636.hddf5 not included
• The main purpose of the library is for visualization. However, the authors claim that it is also for “analyzing”. After reviewing the documentation this is not clear to me. It would be helpful to strengthen this claim by including an example where this is the main focus. I could be misunderstanding this however. Please comment on this.

Installation

• Build badge shows “unknown”
• The “gallery” link is broken
• I tested for Windows and installation works.

Python Package

• Some functions (for example tp.setup.vasp.gen_ibz) show the first line highlighted in gray in the docs.

Repository

• What version are we working with? There is only 1 release (v1.0.0), but the title says 3.0.0 (?). It would be great to explicitly mention the current version available and also using more clear title names for future releases (v1.0.0 instead of “Launch day”).

Tests
Tests are good. They run smoothly with 98% and 89% coverage for calculate.py and settings.py, respectively. Good job.

[kbspooner]

Hi @enricgrau,

Thanks for the review!

Manuscript

• I have expanded this sentence to give a more rigorous explanation of how it can help and how it might have advantages over similar technologies. It now reads "Thermoelectric materials, which convert heat into electricity, could be an important renewable energy source to help slow the encroaching climate crisis, not only by displacing fossil fuels, but by recycling waste heat, which makes up around 50 % of generated energy [@Firth2019]." While I don't give specific examples, I hope using a more concrete statistic makes this more suitable, otherwise like you say it may be better to leave it out entirely from what is ultimately a software paper.
• I've now added reference to the former name, as you spotted, and while those papers did use a pre-release version, as there is no evidence of this it seems fair to remove them.
• There are two libraries, BoltzTraP and BoltzTraP2, which I've now separated for clarity.
• Yeah the last sentence is pretty poor, I've rephrased it "In the future, ThermoParser could be expanded to include an increased number of analysis types and supported codes. On top of this, support for uploading experimental data into the ThermoParser format, including the appropriate metadata, could allow easier comparison of theoretical and experimental results.
• The acknowledgements are now largely converted to CRediT.

Documentation

Gallery

• The links are now fixed.
• The reference has now been formatted.

Tutorials

• This is a (particularly) good point, I have now added introductory paragraphs to each of the tutorials that produce plots, as well as a description of the research behind the data provided in the main examples page.
• I forgot to rebuild the docs after updating them - the links should be fixed now.
• The kappa files are very large and are instead located in a Zenodo repository. As noted in the other review the instructions on accessing these weren't very clear, so the download script is now located in the base examples directory, where the files will be made accessible for all examples and tutorials.
• I cannot reproduce raise ValueError(f"{v!r} is not a valid value for {k}") ValueError: "'#f0901f'" is not a valid value for color, but perhaps I've fixed it in the mean time. Could you try again please?
• The formatting is now in the right language.
• While it is true that much of the code is dedicated to plotting figures for a human to analyse, I think certain parts move closer to analysing, although it depends on where you draw the line. The package ranges from pure plotting (e.g. phonon dispersions), to basic/ routine calculations (ZT calculations and plots), to more bespoke/ involved calculations which approach analysis. For example, the avg-rates plot uses knowldege of the Boltzmann transport equation to collapse a large amount of data ((q-mesh_x x q-mesh_y x q-mesh_z x n_bands x s_mechanisms x temperatures) points) into a handful of lines by weighting by the derivative of the Fermi-Dirac distribution wrt energy, which does much of the analysis legwork. While less complicated, the kappa-target plot reverses the normal thermoelectrics workflow to determine if the most expensive calculations are even worth doing, Ultimately a human must interpret the data, but some of these modules make a significant contribution in my opinion.

Installation

• Looks like these and several previous error have arisen from not regenerating the docs properly after updates. I have now fixed them.

Python Package

• This seems to occur due to using multiple lines in the opening sentence of the docstring. I have replaced two instances of this problem in the setup/vasp.py and one in calculate.py.

Repository

• I haven't been diligent incrementing the version with each update, so the current version is, to a large extent, guesswork. I'll try to do better in the future but there is little I can do retroactively.

Tests

• I realise this wasn't a criticism, but I took the opportunity to make the tests more comprehensive anyhow.

I hope this addresses your problems and concerns, of course if you spot anything else or have more comments, I'll be happy to address them!

For the record, this is in relation to this JOSS review.

[enricgrau]

Thank you for your comprehensive response. I’m quite satisfied with the changes you’ve made so far. I like the current state of the documentation and the manuscript. Here are some additional minor observations.

Installation
This time I came across some minor issue with the commeand pip install . with this error:

ModuleNotFoundError: No module named 'matplotlib'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for tp
Failed to build tp
ERROR: Could not build wheels for tp, which is required to install pyproject.toml-based projects

I fixed it with the following command before retrying: pip install setuptools wheel

I’m now in a different machine, so I’m not sure it’s a local issue but I tend to believe this. Because of this, and as it is an easy fix, I don’t think is a necessary fix for being accepted. You may have a quick look into this.

Data
I was now able to download the data. However, I was only able to do so because you mentioned it here. I did not find any explicit mention on where or how to downloading the data in the documentation. I strongly recommend you include just a small instruction to do this (either manually or/and with the included in script). I might have missed this however, but it just was not clear to me.

Tutorials
I only had problems with tutorial 2. The script keeps running infinitely and I get the following error every couple of seconds (this I was able to reproduce in both of my machines):

  File "C:\Users\User\AppData\Local\Programs\Python\Python311\Lib\multiprocessing\spawn.py", line 140, in _check_not_importing_main
    raise RuntimeError('''
RuntimeError:
        An attempt has been made to start a new process before the
        current process has finished its bootstrapping phase.

        This probably means that you are not using fork to start your
        child processes and you have forgotten to use the proper idiom
        in the main module:

            if __name__ == '__main__':
                freeze_support()
                ...

        The "freeze_support()" line can be omitted if the program
        is not going to be frozen to produce an executable.

        To fix this issue, refer to the "Safe importing of main module"
        section in https://docs.python.org/3/library/multiprocessing.html

Repository
I understand there is nothing you can do retroactively. However, I insist that it would be great to keep this consistent in the future. I see now that the latest uploaded version is 3.1.2, which is great. I encourage you to make a new release matching the latest real version once the JOSS review is done. This so it incorporates all the changes and it accurately reflects your work and progress.

[kbspooner]

pre-script: I wrote this last week but evidently failed to post it. Sorry!

Glad to hear it!

Installation

I didn't realise in some cases setuptools and wheel weren't installed by default, but now I've added them to the requirements. This fix doesn't break the installation on my end, but if you could check if it works that would be great.

Data

I put that information in the files on github but forgot to also put it in the website docs, this is now corrected.

Tutorials

I can't reproduce this error, but another reviewer also had a (different) problem with the multiprocessing in this example on mac, which he found a solution for. I've implemented it in the latest version of the master branch, so perhaps you could try again?

Repository

I will certainly do this!

[enricgrau]

No problem! Thank you for your hard work. I'll close this issue and recommend for publication.