Closed editorialbot closed 6 months ago
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.05 s (892.4 files/s, 128021.2 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 18 546 781 1652
TeX 2 100 0 1382
YAML 6 10 8 601
reStructuredText 14 300 451 262
Jupyter Notebook 2 0 476 122
CSS 3 8 14 64
Markdown 1 15 0 39
DOS Batch 1 8 1 26
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 48 991 1738 4157
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 973
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1088/0370-1298/68/10/304 is OK
- 10.1103/PhysRevB.14.2239 is OK
- 10.21468/SciPostPhysLectNotes.5 is OK
- 10.1038/s41699-023-00378-0 is OK
- 10.1088/0034-4885/77/12/126401 is OK
- 10.1103/RevModPhys.89.011004 is OK
- 10.1103/PhysRevLett.128.166402 is OK
- 10.1088/1367-2630/ac4126 is OK
- 10.1063/1.1611351 is OK
- 10.1142/9781848160224_0014 is OK
- 10.4007/annals.2009.170.303 is OK
- 10.1103/RevModPhys.91.015005 is OK
- 10.1038/nature12186 is OK
- 10.1103/PhysRevLett.111.185301 is OK
- 10.1103/PhysRevB.101.235312 is OK
- 10.1038/ncomms9629 is OK
- 10.1103/PhysRevB.103.075132 is OK
- 10.1103/PhysRevB.104.184501 is OK
- 10.1038/s42005-019-0151-7 is OK
- 10.1007/BF01342591 is OK
- 10.1038/nature25011 is OK
- 10.1126/science.aao1401 is OK
- 10.1103/PhysRevB.108.205144 is OK
- 10.1103/PhysRevB.96.165150 is OK
- 10.1038/s41586-022-05576-2 is OK
- 10.1007/s10955-014-0992-0 is OK
- 10.1088/0256-307X/26/12/123701 is OK
- 10.1088/1751-8113/47/18/185202 is OK
- 10.1103/PhysRevLett.127.246403 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
I've nearly finished reviewing the JOSS points. In the meantime, I'll provide feedback on what I've tried w.r.t. installing and running. I've left them as comments here, rather than open Github issues. They're all minor points.
The python minimum version isn't explicitly specified in the README, although I can see it's set to 3.10.13 in the environment.yml.
I would typically expect to see this in a setup.py
or pyproject.toml
(the latter being prefered).
There are environment.yml
s in the project root and the code/
directories. Diff gives me a subtle difference w.r.t. a few package versions. I assume one is redundant?
From root:
conda env update --file environment.yml
LibMambaUnsatisfiableError: Encountered problems while solving:
- nothing provides requested _libgcc_mutex ==0.1 main
- nothing provides requested _openmp_mutex ==5.1 1_gnu
.
.
.
Same issue if I follow the instructions:
conda env create -f environment.yml
This could be because of the specific pinning, and the fact that I'm trying to install deps on osx-arm64
. If you specify the base OS so I can test this in docker.
environment.yml
fileenvironment.yml
in dockerenvironment.yml
so it's more robustpip install -r requirements.txt
ERROR: Could not find a version that satisfies the requirement python (from versions: none)
ERROR: No matching distribution found for python
I'm not sure it makes sense to specify python
with no version as a dependency in requirements.txt
Removing python
from the requirements.txt
allows me to install the deps with pip, and run the example band structure.
python
from requirements.txt
My suggestion would be to replace the requirements.txt
with a pyproject.yml
. Something like:
# Note, if not specified setuptools is defaulted to
[build-system]
requires = ["setuptools >= 61.0"]
build-backend = "setuptools.build_meta"
[project]
name = "HofstaderTools"
authors = [{name = "Bartholomew Andrews"}]
readme = "README.rst"
requires-python = ">=3.10"
# Pinned project dependencies
# requirements.txt generated from here
dependencies = [
"jupyter>=1.0.0",
"matplotlib==3.8.0",
"numpy==1.26.1",
ADD MORE
]
# TO ADD
[project.urls]
[tool.setuptools.packages.find]
where = ["code"]
# Pinned project dependencies for development
# requirements-dev.txt generated from here
[project.optional-dependencies]
dev = [
"pytest>=7.2.0",
"pytest-cov>=4.0.0",
"sphinx>=7.2.6",
"sphinx-rtd-theme==2.0.0",
]
requirements.txt
with pyproject.yml
which also makes the project installable. I also notice that the CI is explicitly specifying pip packages instead of using pip install -r requirements.txt
to install. I would change this to remove duplication, and keep everything in sync.
requirements.txt
or pyproject.yml
, rather than explicitly restate each python dependencyOne has to set the $PYTHONPATH
for the package to be found. This could be avoided if the package is made installable with the pyproject.toml
.
export PYTHONPATH=$PYTHONPATH:~/HofstadterTools/code
assumes an installation location. I'd add a comment or make this pseudocode:
# Please prepend the path accordingly:
export PYTHONPATH=$PYTHONPATH:path/to/HofstadterTools/code
(as I stupidly just copy-paste on first attempt at running the tests).
$PYTHONPATH
snippet (or make project installable)Trivial point: Tests depend on files using relative paths. This prevents me running pytest in code/
.
This could be made more robust by defining the absolute path. For example:
test_dir = os.path.abspath(os.path.dirname("__file__"))
filename = os.path.join(test_dir, 'butterly', 'butterfly_triangular_q_97_t_1.npz')
A suggestion for future improvements (beyond this JOSS submission): The functions in models.py
would benefit from unit testing.
Documentation deployment is automated to Github pages, which is really nice. Documentation is compact and thorough. An explicit link to the documentation in the README would be benefical, and/or a note on how to build sphinx locally.
cd docs/
sphinx-apidoc -o source ../code
make html
worked for me.
JOSS doesn't specify to comment on this, but whilst checking the code documentation I had a look at the Hofstadter class. This could be simplified by making Hofstadter class a base class that doesn't implement unit_cell
, and implements hamiltonian
method with the default definition (hence allowing to overload where appropriate). Each lattice can then be a child class of Hofstadter:
class Square(Hofstadter)
def __init__(self, p, q, a0=1, t=None, alpha=1, theta=(1, 3), period=1):
super.__init__(p, q, a0, t, alpha, theta, period)
def unit_cell(self):
# lattice vectors
a1 = self.a0 * np.array([1, 0])
a2 = self.a0 * np.array([0, 1])
avec_val = np.vstack((a1, a2))
# basis vectors
abasis1 = self.a0 * np.array([0, 0])
abasisvec_val = np.array([abasis1])
# lattice vectors (MUC)
aMUC1 = a1
aMUC2 = self.q * a2
aMUCvec_val = np.vstack((aMUC1, aMUC2))
# reciprocal lattice vectors (MUC)
bMUCvec_val = fm.reciprocal_vectors(aMUCvec_val)
# symmetry points
GA = np.array([0, 0])
Y = np.array([0, 0.5])
S = np.array([0.5, 0.5])
X = np.array([0.5, 0])
num_bands_val = len(abasisvec_val) * self.q
return num_bands_val, avec_val, abasisvec_val, bMUCvec_val, sym_points_val
def hamiltonian(self, k_val):
return fm.BasicSquareHamiltonian(self.t, self.p, self.q, k_val, self.period)
...
etc
Or one could follow a factory pattern.
Also, another future suggestion: No type-hinting has been used for function args. Adding them would improve readability and really help code static analysis.
One might consider a release on pypi.
I've finished reviewing. The paper's well-written and the code is compact, with good documentation on usage. Once the points outlined here are addressed (as well as the one trivial issue I opened in the repo), I'm happy to approve the paper 🥳
@AlexBuccheri Thank you for catching these issues! I will work on them as soon as possible. In the meantime, to answer your question, this package was tested using Ubuntu 20.04.6 (x86_64). Thank you again for all of your suggestions for improvement!
I tried the conda install in docker for Ubuntu focal with Python 3.10 and had the same problem (using native ARM instead of emulating X86). I also tried dropping extensions of the form =py310h06a4308_0
, but reconciling the env took forever/was hanging.
I'm not sure if the problem stems from pinning python version, if the pinned versions are hardware + OS specific, or both. However, this does imply that the pinning is too specific to make this portable. If you'd like to retain the environment.yml as a general installation method, perhaps you can regenerate the file with only the package names and versions, not the build strings.
conda env export --from-history > environment.yml
removes the build strings (but also annoyingly drops most of the pinning).
@RMeli Do you have experience with creating portable conda environment files?
@AlexBuccheri thank you for looking into this. I doubt such file is portable. When I use conda list --explicit
to export my environment, usually the platform is also included (linux-64
, osx-arm64
, noarch
, ...). I think it would work only if all the packages are marked as noarch
, otherwise the dependencies are hardware and OS specific. Did it work if you use x86 emulation? @bartandrews which command did you use to export your environment?
If the software is supposed to work on macOS/Windows I think a better approach is to have a conda environment listing only the required dependencies (and eventually version pins, where needed). Otherwise, clearly state that the package only works on Linux.
Even better, of course, as suggested in https://github.com/openjournals/joss-reviews/issues/6356#issuecomment-1947166385, would be to properly package the software so that pip
will automatically take care of dependencies. The supported platforms can also be listed in setup.py
/pyproject.toml
.
@AlexBuccheri @RMeli My mistake, I believe that I used the command conda env export > environment.yml
without the --from-history
flag. I will fix this, and I will also investigate the better options that you suggested for package installation. Thank you again for catching this.
I have now replaced the environment.yml
file with the one generated using the --from-history
flag. This is a temporary solution, while I implement a better installation method.
I second @AlexBuccheri 's suggestion with regards to installation in https://github.com/openjournals/joss-reviews/issues/6356#issuecomment-1947166385. Using pyproject.toml
seems like a good idea. I am on Ubuntu 22.04 with conda channel_priority
set to strict
. When I try conda env create -f environment.yml
(with the newest environment file), the solver is unhappy and fails because
LibMambaUnsatisfiableError: Encountered problems while solving:
- package sphinx-5.3.0-pyhd8ed1ab_0 is excluded by strict repo priority
While the command succeeds with flexible
or disabled
channel priority, it's probably not ideal to require a channel priority change to have the conda environment install correctly.
~@katherineding given that the error says sphinx-5.3.0-pyhd8ed1ab_0
, are you trying to use the old environment.yml
or the new one, which does not contain hashes?~ Sorry, I didn't read carefully enough.
@katherineding Thank you for letting me know. Considering that not even my hand-written environment.yml
file was robust, I have decided to drop the environment.yml
option entirely, and stick with pyproject.toml
. I now have updated my installation instructions accordingly.
@AlexBuccheri Thank you again for your comments and review. For some reason, I cannot click on your check-boxes. So I will summarize my responses here:
Installation From your comments, I realize that environment.yml
is not really portable (I have tried both with the --from-history
flag and hand-written, both did not work as intended). Therefore, I have replaced environment.yml
and requirements.txt
files with a pyproject.toml
, as recommended. This seems to work nicely. ~One concern that I have is with the package python3-tk
. This is required in order to have an interactive GUI backend for matplotlib
, and view the plots (crucial for my package), but I am not sure how to list this a requirement in my pyproject.toml
. It cannot be installed via pip, it needs to be installed with python, similar to pip itself. I think that some installations of python come bundled with pip and tk as standard, but it's still worrying that for some people it will not flag that this dependency is missing.~
I have also changed my CI to read from pyproject.toml
.
Tests I have made my project installable, as above. I have also made the test paths absolute, so that pytest
will work in the project directory or src
directory. Note that I have now implemented a src layout, for readability.
Documentation An explicit link to the github pages is in the 2nd bullet point of the readme. I use my custom URL https://hostadter.tools One question that I had: I wanted to make my custom URL link directly to my github pages documentation, however the only way that I could think of to do this is to name my repo with the ".github.io" suffix. Do you know if there is a better way?
I have also included instructions for building the docs locally in the docs
part of the Directory Structure
section of the readme. I use make clean html
, instead of your sphinx-apidoc -o source ../code
. I presume that this is equivalent?
Design Thank you for these suggestions. The Hofstadter sub-classes would certainly rid me of the ugly if
statements. I have not implemented anything here yet. I need to think about how best to approach this. I may implement other models (related to the Hofstadter model) in the future, so I need to think about design going forward.
Distribution I have a test PyPI distribution ready to go now. I am still testing a few things before pushing to the actual PyPI. ~In particular, I am looking for ways to solve this python3-tk
issue...~
Some other changes that I have made:
In summary, I have implemented all of your suggestions, apart from the design section (for now), and I am waiting to check some things with the installation before I push to actual PyPI.
UPDATE: I think that I have fixed my python-tk
issue. It turns out that I do not need python-tk
, which gives me the TkAgg backend (in place of non-interactive Agg). Instead I can use the QtAgg backend, which can be installed via pip using pip install QtPy5
. The full list of interactive backends for matplotlib is here: https://matplotlib.org/stable/users/explain/figure/backends.html#interactive-backends
Therefore, I have now added QtPy5 as a core dependency to my pyproject.toml
. Interestingly, even though they claim that QtPy5 supports Python>=3.7 (https://pypi.org/project/PyQt5/), for me it only worked for Python>=3.9. So I bumped up my requirements accordingly. Now the band_structure
program should reliably produce an interactive 3D plot.
@AlexBuccheri @katherineding Following some additional testing, I have now added a PyPI release (https://pypi.org/project/HofstadterTools/) and have updated the readme. I have checked that this works on Ubuntu for venv environments with Python 3.9, 3.10, 3.11, 3.12. I have also checked that it works in conda environments, although conda throws an unusual libGL error
, which as far as I can tell is an issue with Conda and not HofstadterTools. Please let me know if this is also robust now on your computers. Thanks again!
I have found a possible solution to my ".github.io" suffix question. As far as I know, I can only point custom URLs to github repositories with this suffix. However, instead of building the documentation in the same repository into a different branch, I can configure the CI to deploy the built documentation into a different repository (within the HofstadterTools organization). @RMeli Is it okay if I rename the repository from "HofstadterTools.github.io" to "HofstadterTools"? My plan is to have 2 repos in the HofstadterTools organization: "HofstadterTools" containing the project (like it is now), and "HofstadterTools.github.io" containing the built documentation hosted using github pages.
Sorry @bartandrews, I didn't catch this issue before. I should inquiry how renaming a repository during review works, I'll let you know.
However, I'm not entirely sure why two repositories are needed. http(s)://<organization>.github.io
is the top-level website for an organisation. However, documentation can usually live within the repository under http(s)://<organization>.github.io/<repository>
(i.e. this is a project-specific site.
An explicit link to the github pages is in the 2nd bullet point of the readme. I use my custom URL https://hostadter.tools/ One question that I had: I wanted to make my custom URL link directly to my github pages documentation, however the only way that I could think of to do this is to name my repo with the ".github.io" suffix.
Looking at GitHub Documentation, however, it seems that you can override the default project domain by adding a custom domain to the individual repository.
I think the problem here is that you have put your code into HofstadterTools.github.io, which is technically supposed to be the repo for the organisation website. So renaming it to HofstadterTools
should allow you to have a project-specific website normally under http(s)://HofstadterTools.github.io/HofstadterTools
(which should be possible to override).
I just checked, and there is no problem changing the repository during review, if needed.
Thank you for letting me know! I will think about this... Most probably, I will change it, as you say, because it is non-standard to have the code in "HofstadterTools.github.io". I think "HofstadterTools" makes more sense
it is non-standard to have the code in "HofstadterTools.github.io". I think "HofstadterTools" makes more sense
Yes, I agree. But up to you how you want to proceed here.
@editorialbot set https://github.com/HofstadterTools/HofstadterTools as repository
I'm sorry @bartandrews, I'm afraid I can't do that. That's something only editors are allowed to do.
@RMeli I have now renamed the repository to HofstadterTools
. After some digging, I think that if the repo name is HofstadterTools
, then the best custom URL I can have is https://hofstadter.tools/HofstadterTools
. I think that the apex domain alone https://hofstadter.tools
is no longer possible (please correct me if I'm wrong). Therefore, I have implemented the 2 repo solution, so that the apex domain https://hofstadter.tools
can point directly to my documentation.
The new code repo location is: https://github.com/HofstadterTools/HofstadterTools
@editorialbot set https://github.com/HofstadterTools/HofstadterTools as repository
Done! repository is now https://github.com/HofstadterTools/HofstadterTools
Hi @bartandrews, just to say that I have been at a workshop this week, but have seen the thread pings and will have a proper look on Monday.
@bartandrews
Thanks for the positive response w.r.t. all the feedback. The changes look great! Just to follow up on those last points:
Installation
Installation works smoothly for me with python 3.11 on Mac. Tests work out of the box, as does installing docs dependencies and building 🙌🏻
Documentation
make clean html
is equivalent to the command I suggested, and the better choice. W.r.t. documentation would just add this comment after pip install -e ".[docs]"
in the top-level README:
To build the documentation, run:
make clean html
in the docs/
folder.
But this is super-trivial and non-blocking. I'll leave it to your discretion.
W.r.t. the documentation deployment issues you were having, I would not have been helpful as the main project I'm currently associated with runs on Gitlab, with custom buildbot CI (sigh). But it's great that you found a soluton and @RMeli added some context - I made a note for my own reference 😄
I assume I can run this command:
@editorialbot recommend-accept
I'm sorry @AlexBuccheri, I'm afraid I can't do that. That's something only editors are allowed to do.
Ah, I cannot run it haha. Ok @RMeli , this review is done for me. @bartandrews has thoroughly addressed all my points and I'm happy to approve. Let me know if I need to run a different command.
@AlexBuccheri Thank you again for catching those issues, and for your suggestions for improvement. I'm glad that the package is more robust now and works on Mac OS 🎉 I will update the readme as recommended, when I next get the chance
Thank you very much @AlexBuccheri for the in-depth review, it's really appreciated! Your recommendation is noted!
I have finished reviewing the code repository and paper submission. The code is well-documented and clean. This package addresses a clear need in the topological band structure community for an accessible way to perform Hofstadter and Hofstadter-adjacent tight-binding model calculations, particularly with associated quantum geometry and topology information. The revisions suggested by @AlexBuccheri have already addressed the bigger issues relating to installation, packaging, and testing.
I verified that basic installation, advanced installation, running tests, and building local documentation succeeds on my Ubuntu 22.04, Python 3.11 system.
I am delighted to recommend acceptance! :partying_face:
Below, I list some minor suggestions that I think will improve the user experience of this package. They are non-blocking, however, so please feel free to ignore them.
venv
with HofstadterTools
. This would save people a google if they're not familiar with venv
. (I for one needed to look this up!) If I recall correctly, these instructions were present in an older version of the docs. I would simply add after the sentence "To avoid dependency clashes, we recommend the use of a python virtual environment, such as venv
." something like:To use
venv
, create a virtual environment:python -m venv /path/to/env
and activate it:source /path/to/env/bin/activate
. Then you may install HofstadterTools into the newly activated virtual environment by runningpip install HofstadterTools
. This environment now would only contain HofstadterTools and its dependencies.
source /path/to/env/bin/activate
must be run each time a new terminal shell is opened before HofstadterTools can be used.To switch virtual environments. Run
deactivate
then source a different desired environment.To remove HofstadterTools and its dependencies, run
deactivate
and remove the entire/path/to/env
folder.
Consider making more explicit (in documentation/help message) the difference in functionality between band_structure
and butterfly
. I am guessing that mainly band_structure
considers one specific field strength (and also produces quantum geometry information), while butterfly
runs through an array of field strengths.
Consider documenting the meaning of the high symmetry points "Y" and "S" in output of Hofstadter.unit_cell()
. "S" in particular does not seem to be standard notation, please correct me if I'm wrong on this.
JOSS requires community guidelines for third parties wishing to 2) Report issues or problems with the software and 3) Seek support. Currently the README only has a "Contributing" section where its probably implicit that support requests should go to @bartandrews. It would probably be good to add a few explicit words about reporting issues and seeking support (e.g. if you would prefer people use the github issues/discussions system v.s. directly contact you).
@katherineding Thank you for your suggestions for improvement! :pray: I am grateful for your feedback regarding the installation, the issue that you spotted https://github.com/HofstadterTools/HofstadterTools/issues/4 , and also the conda-forge / documentation advice above. I will implement these changes as soon as possible
Thank you @katherineding for the reviewing this submission, it's really appreciated! @bartandrews please ping me when you are done with implementing the final suggestions so that I can start the final checks.
Hi @bartandrews, how is the implementation/integration of the latest suggestions going? Please let me know if you have any questions and please ping me when you are done.
@RMeli Thanks for your message. I am almost done, just working on the final details. I estimate that I will be finished by Monday or Tuesday. (I'm at the APS March Meeting this week, which is the reason for the delay.)
@katherineding Thank you again for your feedback and suggestions for the code. It is much appreciated! :smiley: I append my responses below:
Packaging
Documentation
band_structure
and butterfly
programs in the Quick Start section and in the tutorials.I have added a note in the Theory>Band Structure section of the documentation explaining how the paths are computed. This comment also made me think more generally about the paths in the 2D band structures. Since it is not possible to define a high-symmetry path in the generalized Hofstadter model that is a continuous function of lattice anisotropy and obliqueness, I had 2 options: (1) Write an algorithm to compute a high-symmetry path (if such a path even exists) on a case-by-case basis, or (2) define a reference path through k-space that will not always be a high-symmetry path. I decided on (2) because I couldn't think of an intuitive way of implementing (1), and I'm not convinced that the paths from (1) would always be informative, e.g. for lattices with few symmetries. I have now changed the x-axis label on the 2D band structures from "symmetry path" to "reference path" to reflect this, and I have made it clear in the documentation that the reference paths used are for easy comparison and do not always correspond to high-symmetry paths. I am open to any suggestions that you have.
NB: I use $\Gamma\to X \to M \to Y \to \Gamma$ instead of the standard $\Gamma \to X \to M \to \Gamma$ for the square lattice because I define these points w.r.t. the MUC reciprocal lattice vectors and the former more often captures the key properties of the band structure in the presence of a magnetic field. See, for example, band_structure -disp both -lat square -nphi 1 2
. Similarly for the triangular lattice, etc.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Submitting author: !--author-handle-->@bartandrews<!--end-author-handle-- (Bartholomew Andrews) Repository: https://github.com/HofstadterTools/HofstadterTools Branch with paper.md (empty if default branch): Version: 1.0.3 Editor: !--editor-->@RMeli<!--end-editor-- Reviewers: @AlexBuccheri, @katherineding Archive: 10.5281/zenodo.10809833
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
@AlexBuccheri & @katherineding, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @RMeli know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @AlexBuccheri
📝 Checklist for @katherineding