search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: QMix: A Python package for simulating the quasiparticle tunneling currents in SIS junctions #1231

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @garrettj403 (John Garrett) Repository: https://github.com/garrettj403/QMix/ Version: v1.0.3 Editor: @labarba Reviewer: @FaustinCarter, @josephhardinee, @PaulKGrimes Archive: 10.5281/zenodo.2538162

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/00018094ad4ceb3165ed9515e6f912a4"><img src="http://joss.theoj.org/papers/00018094ad4ceb3165ed9515e6f912a4/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/00018094ad4ceb3165ed9515e6f912a4/status.svg)](http://joss.theoj.org/papers/00018094ad4ceb3165ed9515e6f912a4)

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) 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

@FaustinCarter & @josephhardinee & @PaulKGrimes, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @FaustinCarter

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @josephhardinee

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @PaulKGrimes

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @FaustinCarter, it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

labarba commented 5 years ago

👋 @FaustinCarter, @josephhardinee, @PaulKGrimes — This is where the action happens. You can read about the JOSS Review Process in our documentation. Feel free to ask any questions here, and to open issues in the target repo, as needed.

FaustinCarter commented 5 years ago

I've had a go at installing it and had some issues. I'll drop them here now so they can be addressed in parallel with other things.

Attempts at installation:

  1. I can't use the included environment.yml file on Windows. I'm not sure if this is Windows-specific, but the yml file specifies the exact hash to use for each package version and those don't match the ones I see when I do conda search foo. Recommendation is to just specify version numbers and let conda sort out which system-specific repo it should pull from.
  2. Straight-up pip install in fresh conda environment (with just python 3.7) failed also because of error trying to read in the long description in setup.py from README.md, which has some non-readable characters (i.e. images?). This suggests the code on PyPI is not the same as the latest code in master on github. However, they both have the same version number, 1.0.0. Recommend bumping the version to 1.0.1 and reuploading to PyPI (and also creating a new release in github to trigger a Zenodo update).
  3. Trying pip install -e from source... Success! So the current code installs fine after a download and local build.

I also ran the tests, which all passed on the first go-around. You might mention in the documentation for the tests that pytest is a dependency that needs to be separately installed in order to use.

Another issue that I see right away is that the documentation doesn't have an API section that contains all the class and function docs. I scanned through the major class docstrings and they appear to be generally in good shape, so I think you can get this most of the way there with Sphinx's autodoc feature.

OK, that's all for now. More as I work through the code base. Once the installation issues are fixed I'll tick that box above in the review.

PaulKGrimes commented 5 years ago

John has fixed the environment.yml file, and replaced it with environment-qmix.yaml in the repository. The environment creation is currently working on both my Windows 10 Anaconda installs, miniconda on Ubuntu WSL and Anaconda on CentOS 7, although I have to run python setup.py install from the qmix directory after creating the environment.

The pip installer issue looks like an issue with non-standard quotation marks and hyphens in the second reference in README.md - that's often a risk when cutting and pasting references. The pip install method works on Ubuntu WSL and CentOS, so this is a Windows specific issue.

Paul

On Tue, Feb 5, 2019 at 12:08 AM Faustin Carter notifications@github.com wrote:

I've had a go at installing it and had some issues. I'll drop them here now so they can be addressed in parallel with other things.

Attempts at installation:

  1. I can't use the included environment.yml file on Windows. I'm not sure if this is Windows-specific, but the yml file specifies the exact hash to use for each package version and those don't match the ones I see when I do conda search foo. Recommendation is to just specify version numbers and let conda sort out which system-specific repo it should pull from.
  2. Straight-up pip install in fresh conda environment (with just python 3.7) failed also because of error trying to read in the long description in setup.py from README.md, which has some non-readable characters (i.e. images?). This suggests the code on PyPI is not the same as the latest code in master on github. However, they both have the same version number, 1.0.0. Recommend bumping the version to 1.0.1 and reuploading to PyPI (and also creating a new release in github to trigger a Zenodo update).
  3. Trying pip install -e from source... Success! So the current code installs fine after a download and local build.

I also ran the tests, which all passed on the first go-around. You might mention in the documentation for the tests that pytest is a dependency that needs to be separately installed in order to use.

Another issue that I see right away is that the documentation doesn't have an API section that contains all the class and function docs. I scanned through the major class docstrings and they appear to be generally in good shape, so I think you can get this most of the way there with Sphinx's autodoc feature.

OK, that's all for now. More as I work through the code base. Once the installation issues are fixed I'll tick that box above in the review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1231#issuecomment-460514835, or mute the thread https://github.com/notifications/unsubscribe-auth/AafsycG5tnT5XwEPz21DYZQwRWae13mpks5vKRHmgaJpZM4ah6f3 .

-- Receiver Laboratory Smithsonian Astrophysical Observatory 60 Garden St, Cambridge, MA 02138 +1 (617) 496-7640

garrettj403 commented 5 years ago

Hi @FaustinCarter, thank you for your feedback.

As Paul mentioned, I have updated the environment file so that it should work on Windows now. I was previously using conda env export > environment.yml to generate the environment.yml file, but this doesn't work across different platforms. I have rewritten the environment to only include the necessary packages without specifying versions or build codes.

I've also fixed the issue with the setup.py file. I was previously reading in README.md in order to generate the description for setup.py, but README.md contained some non-ASCII characters. I have now rewritten the description so that this doesn't happen anymore.

I'll take a look at adding the API for the functions and classes ASAP.

garrettj403 commented 5 years ago

Hi @FaustinCarter, I have added docs for all of the functions and classes here. This includes an index here and a module index here.

PaulKGrimes commented 5 years ago

I have been able to confirm the operation of the experimental data analysis and impedance fitting example using my own experimental data. This appears to work fairly well, and gives results as expected. There could be some enhancements made to the software to make loading data from a variety of file types simpler - right now the data has to be reformatted to the style of data file used by John.

I have also been able to recreate some of the figures from my PhD thesis, calculating the mixer conversion gain and pumped IV curves for a 4 tone mixer (LO, USB, LSB and IF tones) which used a completely different multitone harmonic balance code, written by myself. John and I use somewhat different definitions of mixer gain (particularly around the available power at the IF), but the calculated mixer gains are similar. To me, this confirms that the software is operating broadly as expected, and accurately calculating the behavior of the SIS junction when used as a superheterodyne mixer.

However, the QMix code is extremely slow for 4 tones, taking approximately 5 hours to calculate the mixer currents using the standard response function in the multitone examplte. The 3 tone version of the calculation (dropping the lower sideband) finishes in about 3 minutes. I have some ideas about how this performance might be improved, but I don't think that that is necessary at this time since no claims are made about the performance of the software.

I think the only box that I'm not ready to tick is the Community Guidelines box.

garrettj403 commented 5 years ago

Hi @PaulKGrimes, thank you for all your work validating the code. I am interested to hear about how to improve the performance of the 4 tone simulations, but we can talk about that later.

With respect to the Community Guidelines, I added more information to the CONTRIBUTING.md file. I also added a CODE_OF_CONDUCT.md (generated using GitHub's template) and some CODE_GUIDELINES.md.

garrettj403 commented 5 years ago

@whedon set v1.0.1 as version

whedon commented 5 years ago

I'm sorry @garrettj403, I'm afraid I can't do that. That's something only editors are allowed to do.

garrettj403 commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

FaustinCarter commented 5 years ago

I'm very impressed in general with this package. It does a lot of SIS stuff very neatly, and presents the results in a readable way. I have a few more comments/suggestions and once those are addressed I'll be ready to check the final boxes:

General documentation

There are a lot of helper functions (resp_swap, for instance) that do specific things, but I don't immediately see why they would be needed. I expect that this is because I am not an SIS mixer expert, but I imagine some new grad student might also not appreciate them fully. It would be good to include at least a minimal use case for all helper functions (i.e. this is useful for doing the following thing). If it is a function the user is not ever really supposed to use (i.e. just for internal Class calculations), consider prepending the function name with an underscore

There are several different response function types. It might be useful to say something about when/why each one ought to be used.

There is a lot of SIS specific jargon that I'm not familiar with. One example is the term "photon voltage". Is that the photon energy divided by electron charge, like it sounds, or something else? Probably this is all well understood by field experts, so I'll defer to the field expert as to whether everything was understandable on the first go. You might consider asking a newish grad student to read through your docs and point out all the terms they are unfamiliar with, but I'm not going to hold you to it!

One of the submodules in your exp sub-package is just a dictionary of default parameter values. Although the comments in the code give some explanation, the auto-doc is very garbled. This needs to be fixed. Also, in Examples 1 and 2, there should be some explicit reference to this default parameter list, or all the values for the toy models should be explicitly passed to the constructors. The less that happens under the hood the better for Examples.

Handling user data

In the experimental data sub-package, there is not a clear specification for what data format is required for both RawData0 and RawData classes. There is a link in the docs to some example data files, but those files do not even have column names, so making inferences is hard.

I recommend the following fixes:

Optionally (and for future reference), I think it is good practice to accept a data structure rather than a file into your routines. For instance, if you accept a numpy.ndarray or a pandas.DataFrame or xarray.Dataset or a python dict of iterables, you would give the user the freedom to load up their data in whatever way they like. You can optionally provide a parser that converts your preferred type of files into that data structure. This saves folks from having to load up their data, save their data in your format, and then reload it from file when they run your code.

Plotting

All of your plotting routines should return a handle to the matplotlib figure object at a minimum. It is very likely that a user will want to continue to annotate, or plot extra stuff, or take your plot and stick it into a larger figure, and they can't do any of this easily with a saved pdf.

The typical way of handling this is to accept a handle to a matplotlib axis as an optional keyword argument and then:

There are also probably other sane variations on this theme, and I'll happily accept any of them, as long as the user has some way of getting a matplotlib object back from the plotting routines.

labarba commented 5 years ago

@PaulKGrimes — I see you've checked all your review items. Are you ready to recommend publication?

labarba commented 5 years ago

👋 @josephhardinee — Your checklist is still untouched. Have you been able to get started with this review?

garrettj403 commented 5 years ago

Hi @FaustinCarter, thank you for your detailed review.

I think that I have addressed all of your points now.

General documentation

There are a lot of helper functions (resp_swap, for instance) that do specific things, but I don't immediately see why they would be needed. I expect that this is because I am not an SIS mixer expert, but I imagine some new grad student might also not appreciate them fully. It would be good to include at least a minimal use case for all helper functions (i.e. this is useful for doing the following thing). If it is a function the user is not ever really supposed to use (i.e. just for internal Class calculations), consider prepending the function name with an underscore

There are several different response function types. It might be useful to say something about when/why each one ought to be used.

There is a lot of SIS specific jargon that I'm not familiar with. One example is the term "photon voltage". Is that the photon energy divided by electron charge, like it sounds, or something else? Probably this is all well understood by field experts, so I'll defer to the field expert as to whether everything was understandable on the first go. You might consider asking a newish grad student to read through your docs and point out all the terms they are unfamiliar with, but I'm not going to hold you to it!

One of the submodules in your exp sub-package is just a dictionary of default parameter values. Although the comments in the code give some explanation, the auto-doc is very garbled. This needs to be fixed. Also, in Examples 1 and 2, there should be some explicit reference to this default parameter list, or all the values for the toy models should be explicitly passed to the constructors. The less that happens under the hood the better for Examples.

Handling user data

In the experimental data sub-package, there is not a clear specification for what data format is required for both RawData0 and RawData classes. There is a link in the docs to some example data files, but those files do not even have column names, so making inferences is hard.

I recommend the following fixes:

* add column names to the csv files, or at least include them in the readme file (along with expected units)

* Explicitly spell out what the expected format is for any data files that the user might want to read in.

Optionally (and for future reference), I think it is good practice to accept a data structure rather than a file into your routines. For instance, if you accept a numpy.ndarray or a pandas.DataFrame or xarray.Dataset or a python dict of iterables, you would give the user the freedom to load up their data in whatever way they like. You can optionally provide a parser that converts your preferred type of files into that data structure. This saves folks from having to load up their data, save their data in your format, and then reload it from file when they run your code.

Plotting

All of your plotting routines should return a handle to the matplotlib figure object at a minimum. It is very likely that a user will want to continue to annotate, or plot extra stuff, or take your plot and stick it into a larger figure, and they can't do any of this easily with a saved pdf.

The typical way of handling this is to accept a handle to a matplotlib axis as an optional keyword argument and then:

* if they don't pass in their own axis, just create a new figure, plot stuff up, and return the figure.

* if they do pass in their own axis, plot directly on the axis passed in

There are also probably other sane variations on this theme, and I'll happily accept any of them, as long as the user has some way of getting a matplotlib object back from the plotting routines.

Thank you again and please let me know if I missed anything!

PaulKGrimes commented 5 years ago

Hi Lorena,

Yes, I'm ready to recommend publication.

Paul

On Sun, Mar 10, 2019, 15:56 Lorena A. Barba notifications@github.com wrote:

@PaulKGrimes https://github.com/PaulKGrimes — I see you've checked all your review items. Are you ready to recommend publication?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1231#issuecomment-471362780, or mute the thread https://github.com/notifications/unsubscribe-auth/Aafsybi67X2IS2LFIuDSOcYzNsH88IXwks5vVY20gaJpZM4ah6f3 .

josephhardinee commented 5 years ago

I am ready to recommend publication. There is one minor issue with software version being off a micro version, but that is just due to updates to the package in response to reviewer comments (1.0.0 to 1.0.2). I'm clicking the checkbox on versions match as I assume the micro versions don't matter as much until final proof is ready.

The documentation is quite nice, with several examples showing usage in the documentation as well as separate notebooks in the repository. The package installed using pip into my general environment with no issues. All tests passed out of the box as well. The contributor guidelines are thorough (code of conduct, contributing, style guidelines, etc). Overall it looks like a pretty good package and installation and usage was straightforward with not even any minor hiccups.

labarba commented 5 years ago

At this point, we just wait on you, @FaustinCarter, to approve the author's last changes, and to tick of the remaining items in your checklist. Please let me know here when you're ready to recommend publication.

FaustinCarter commented 5 years ago

I am very happy to recommend publication. This is a really excellent job!

garrettj403 commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

labarba commented 5 years ago

@garrettj403 Can you delete the last sentence? The JOSS paper format includes links to the software repository and its archive on the right margin of the first page. We don't include the archive as a citation, but create linked DOIs with the Crossref deposit.

labarba commented 5 years ago

After you do this ☝️, the next step is to create a tagged release (post the version number here), and update your Zenodo deposit to a new version (no need for a new DOI, just a version). Post the archive DOI here.

garrettj403 commented 5 years ago

Hi @labarba, I've removed the last sentence from paper.md and issued a new release. The new version number is v1.0.3 and the Zenodo DOI is 10.5281/zenodo.2538162 (for the archive as a whole).

labarba commented 5 years ago

@whedon set v1.0.3 as version

whedon commented 5 years ago

OK. v1.0.3 is the version.

labarba commented 5 years ago

Please edit the metadata of your Zenodo deposit so the title and author list match the JOSS paper.

garrettj403 commented 5 years ago

Okay, I've updated the Zenodo archive. The title and authors now match the JOSS paper.

labarba commented 5 years ago

@whedon set 10.5281/zenodo.2538162 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2538162 is the archive.

labarba commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/567

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/567, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
whedon commented 5 years ago 

OK DOIs

- 10.1063/1.2424407 is OK
- 10.1103/PhysRev.147.255 is OK
- 10.1063/1.1576515 is OK

MISSING DOIs

- None

INVALID DOIs

- None
labarba commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 5 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/568
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01231
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? notify your editorial technical team...

labarba commented 5 years ago

Congratulations, @garrettj403, your JOSS paper is published 👏

Huge thanks to the reviewers: @FaustinCarter, @josephhardinee, @PaulKGrimes 🙏

whedon commented 5 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01231/status.svg)](https://doi.org/10.21105/joss.01231)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01231">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01231/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01231/status.svg
   :target: https://doi.org/10.21105/joss.01231

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

garrettj403 commented 5 years ago

That's fantastic! Thank you for all your help @labarba. Thank you also @PaulKGrimes, @FaustinCarter and @josephhardinee for your work testing QMix and your helpful comments. Overall, this was a very constructive and pleasant review process. Cheers!