jabirali / bodge

Numerical library for working with clean superconductors in Python using the Bogoliubov-de Gennes formalism
MIT License
2 stars 1 forks source link

[REVIEW comments] for the paper and code submitted to JOSS #4

Closed yw-fang closed 1 month ago

yw-fang commented 1 month ago

For the submitted work https://github.com/openjournals/joss-reviews/issues/7134, I found this code Bodge was and is still being developed by @jabirali with substantial scholarly effort and all the basic features of a scientific code. This code can model many materials systems including the conventional/unconventional superconductors and magnetic systems once the Hamiltonian was properly given. The code is well written and the examples are easy to follow. I think this work basically meet the criteria for the publication at JOSS.

Herein, I list several comments on the manuscript and code itself that you may consider addressing before the accept for publication.

Manuscript

Code

jabirali commented 1 month ago

Dear Dr. Fang,

For the submitted work openjournals/joss-reviews#7134, I found this code Bodge was and is still being developed by @jabirali with substantial scholarly effort and all the basic features of a scientific code. This code can model many materials systems including the conventional/unconventional superconductors and magnetic systems once the Hamiltonian was properly given. The code is well written and the examples are easy to follow. I think this work basically meet the criteria for the publication at JOSS.

Herein, I list several comments on the manuscript and code itself that you may consider addressing before the accept for publication.

Thank you for taking the time to review my work, and for your positive characterization and constructive input on the Bodge project.

While I am also working on the superconductivity (mostly on prediction of high Tc superconducting hydrides), it's dramatically different from the research direction you've moving forward. So I am not an expert in your field. My question on the manuscript is that Bodge was said to be able to study the superconductivity in magnetic heterostructures, could you list several more details properties (e.g. Tc) of superconductivity if Bodge can address in the manuscript? It is not very clear to me what superconducting properties it can study in the present version.

Thank you for the helpful comment. I will try to elaborate a bit on this topic in the paper so that it becomes easier for new users with a background in superconductivity but not in tight-binding models specifically.

As you could see from the tutorial, the current code can be directly used to calculate spectroscopic properties such as the local density of states. It could also easily be extended to obtain e.g. the spin-dependent LDOS.

One thing that is not shown in the tutorial, but is discussed in this paper where we used Bodge for the calculations, is that one can use basically the implemented free_energy function to find the magnetic ground state of a superconducting system (basically: if spins are placed on a superconductor, for what spin orientations is the free energy minimized.). This lets one identify how superconductors affect e.g. the "RKKY interaction".

What is less obvious is that the same code can be used to calculate other observables, notably charge currents and the critical temperature $Tc$. There are many ways this can done. The conventional way is to calculate the eigenvectors of the system using dense matrices (using the diagonalize method in Bodge), and then one can use an equation that depends on this result to self-consistently calculate the gap $∆{ij}$ and currents $\boldsymbol{J}_{ij}$ at lattice coordinates $(i, j)$. Standard equations for this is provided in e.g. Zhu's textbook which I referred to in the manuscript. In a future version of Bodge, I hope to incorporate these equations also directly into Bodge itself.

A more advanced approach is to use Green function methods that can be implemented using sparse matrices, which can be made much more efficient than the diagonalization approach. I have an implementation of this on the development branch of Bodge, but it requires much more documentation and testing before I want to merge it into main (and possibly some generalization to be more useful for others too). Since I'm not actively using that part of the code base right now, it might take some time until it makes it into the main.

Once you know how to calculate the gap $\Delta$, then as you know determining $T_c$ is a matter of finding the lowest temperature at which $\Delta \to 0$. My favorite approach for that is a binary search algorithm. If you're interested in seeing how this can be done, this function on the develop branch implements a $T_c$ calculation for s-wave superconductors based on a parallelized sparse Green function algorithm.

As you might know, this kind of model is however not well-suited for predicting new high-Tc superconductors, since it requires that you already know the "effective electron-electron attraction" as a model parameter. This is in practice a fitting parameter. What it can however be useful for, is designing components such as e.g. "superconducting spin valves", where you can calculate how much $T_c$ changes relatively to its theoretical maximum as a function of e.g. magnetic orientations of adjacent magnets.

This was a lot of details, I'll try to keep it a bit shorter in the manuscript itself :). The current brief summary is that the Bodge on main branch can be used to calculate e.g. currents and $T_c$ as well, but it requires the user to write some code of their own to implement the functions that calculate e.g. the superconducting gap. In the future, I hope to integrate a few more such functions directly into Bodge when I have time.

I'll add some discussion of this to the paper and get back to you.

There is one more minor comment on the manuscript. Although the CSR has been popular as a format, I think it's better to give the full definition in the main text for some readers who are not familiar with it.

Good point, I will add such a clarification to the manuscript.

The test suite needs to be further improved so that the core functions can be tested more thoroughly.

Thank you for your input. I will think about how to best extend the current test suite and get back to you on this, both potential "mathematical" and potential "physical" way to tests the code in a robust way.

Please let me know if you had any suggestions in mind.

The unittest commands are recommended to be shown in README or the documentation.

I have now made the test suite more accessible in two ways:

  1. I have added a brief description of how to run the unit tests under the Development section of the README. (Before, it was described only in the CONTRIBUTING file.)
  2. I already had a continuous integration pipeline via GitHub Actions, which automatically ran all unit tests whenever I pushed new code to the main branch. However, this was not very visible to others. I have now added this "badge" to the top of the README: Tests This shows the test status at a glance, and the image can be clicked to see more details.

You may consider adding the URL of the doc to the upper right side of the code page in Github so that the users can easily find the documentation.

I have now added this badge to the top of the README: Docs This lets the user immediately see a link to the documentation, instead of having to read through parts of the README to find the in-text link.

Summary

I think the remaining tasks on my end are then:

The other points, I think, are now addressed. Please let me know if you have anything more to add, and I'll get back to you once the above is fixed.

yw-fang commented 1 month ago

@jabirali Great! Please remember to revise the manuscript accordingly. I think these responses are satisfactory.

jabirali commented 1 month ago

@yw-fang:

From before, I had a set of unit tests that provided near-100% test coverage, but focused more on "mathematical" tests: checking that symmetries are correct, matrices are Hermitian, that correct exceptions are raised for invalid input, and so on.

I have now extended and documented the existing test suite to reach exactly 100% test coverage, and have in addition added a file test_physics.py which includes 8 new physics-based "integration tests". These new tests validate different combinations of features from Bodge to see that various numerical experiments on superconductors and magnets yield known physical predictions expected from theory.

I will try to revise the manuscript as discussed later this week.

yw-fang commented 1 month ago

@jabirali Great! Thank you for your efforts. I think it's time for me to tick the task of automatic unit test. Thank you!

jabirali commented 1 month ago

@yw-fang: I have now implemented your suggestions in the latest version of the manuscript:

I believe I have now implemented all your requests, and am therefore closing this issue as completed. But of course, feel free to open a new issue if you believe that further improvements are required before proceeding to publication.

yw-fang commented 1 month ago

@yw-fang: I have now implemented your suggestions in the latest version of the manuscript:

  • I added a paragraph at the end of the "Statement of Need" section, describing what sparse matrices (e.g. CSR) are and when/why these provide a performance benefit for numerical simulations;
  • I added several paragraphs with new discussion and references at the end of the "Examples and Workflows" section, which discuss in more detail different ways Bodge can be used in actual calculations: matrix diagonalization vs. matrix expansion, which observables can be calculated with built-in methods vs. if the user writes some code on their own, and some additional notes on how one can approach e.g.

    I c

    and

    T c

    calculations.

I believe I have now implemented all your requests, and am therefore closing this issue as completed. But of course, feel free to open a new issue if you believe that further improvements are required before proceeding to publication.

That's pretty nice! I think the researchers who use this code will be benefited from your recent contributions. Thanks!