JOSS Review: Documentation

[lheagy]

I think there are a number of steps that can be taken in the documentation to help users get up and running more easily. This is connected to the JOSS review at openjournals/joss-reviews#566

• First off, the documentation is built as a single, large page, if this could be split into multiple pages, that would help ease navigation
• Installation instructions: these are in the README, but should also be included in the documentation
• Example usage: in addition to the plots shown in the readme, it would be helpful to include examples which show both the code usage and the resulting plot

@yxqd may also have some suggestions here.

[yxqd]

@jarvist: I agree with @lheagy. For the third point above on "Example usage", a tutorial will be really helpful. And I agree that it would be great if you can expand the documentation site to add sections on installation and usage, and improve the API section.

[jarvist]

These all sound sensible points. I'll see what I can do. Thanks.

[jarvist]

OK, I've had an initial go at this:
https://jarvist.github.io/PolaronMobility.jl/

I'll proof-read & integrate some plots the next time I can make the time.

[lheagy]

Thanks @jarvist, @yxqd: would you also be willing to take a look?

[yxqd]

Thanks @jarvist. The doc is much improved. For the examples, could you please add reference(s) for the parameters used in your calculation? For example, in your first example for CdTe, it says:

α=feynmanalpha(7.1, 10.4, 5.08E12, 0.095)

Please add a reference or two from which these parameters were obtained, maybe a volume of Landolt-Börnstein.

[yxqd]

Thanks @jarvist. The documentation is much improved! Here are some small problems to fix

Installation

Please replace "git://" with "https://". The former fails if there is firewall. It is safer to assume https works for most people.

Examples

• push!(LOAD_PATH,"../src/") # load module from local directory
  Is it necessary if the installation step is already performed? Is this for "developers" rather than "users"?
• @test failed. using Base.Test fixed that. Please add a hint in the documentation
• using PlotPolaron errored with the following message:

   Run `Pkg.add("PlotPolaron")` to install the PlotPolaron package.
   Stacktrace:
    [1] _require(::Symbol) at ./loading.jl:435
    [2] require(::Symbol) at ./loading.jl:405
  

• as suggested by @lheagy , could you add some pictures of typical plots expected for the Temperature-dependent behaviour example?

[jarvist]

Hello everyone - hopefully with this last commit (e4ac60d), all of the issues above should have been solved!

Longer term the package could definitely do with some restructing, breaking down of the fairly monolithic functions, and a complete writing of the PlotPolaron pseudo-package in terms of Plots.jl recipes. But it will be easier to do these more organically while also extending + improving the package.

[yxqd]

@jarvist 👍 with the improvements of documentation on usage and the plots.

The instructions on plotting using PlotPolaron still does not work for me, but the alternative (gnuplot) works.

I feel the part on using PlotPolaron can be made smoother for users. But I am not an expert in Julia, and don't know enough to comment. @lheagy please decide if you want to ask an Julia expert to review or leave it as is. In the latter case, I would suggest @jarvist to make the gnuplot instruction more complete, but it is not required for closing this issue.

[lheagy]

I also could not get PlotPolaron working with the provided instructions.

I think that there is a discrepancy here if you install using
Pkg.clone("git://github.com/Jarvist/PolaronMobility.jl.git")
or if you git clone and then add it to your path.

I do think that plotting is an important part of usability, so I think this is worth sorting out, or at a minimum, expanding the examples to give the user more instructions on using gnuplot as a part of the workflow.

[jarvist]

OK. That all makes sense. I'll try and fix the plotting one-way or another.
What I really need to do is write some Plots.jl recipes, but this I something I do not yet understand how to do.

[wkearn]

Hello all! @Kevin-Mattheus-Moerman pointed me to your discussion here, and I'm happy to offer any advice re: plotting that I can.

This is what I had to do to get your plots to run:

julia> using PolaronMobility, Plots
julia> gr()
julia> include(Pkg.dir("PolaronMobility","src","PlotPolaron.jl"))
julia> using PlotPolaron
julia> ... # Do your polaronmobility calculations
julia> plotpolaron(...)

This will work if you have installed PolaronMobility.jl using Pkg.clone and you have Plots.jl installed. The using Plots and gr() calls are necessary on my machine to get around a world age error, but I'm not sure if that is something wrong here or in Plots. I'll try to track down that bug.

Also when I try to plot the example

julia> MAPIe=polaronmobility(10:10:1000, 4.5, 24.1, 2.25E12, 0.12)
julia> plotpolaron("MAPI-electron",MAPIe)

I get an error in the radius vs temperature plot because MAPIe.rfsmallalpha doesn't have anything in it. The plots before that one work fine, I think, but I don't have enough domain knowledge here to check.

@jarvist if you want some help converting plotpolaron into Plots.jl recipes, let me know (tag me in a new issue or something). At a first glance you aren't doing anything too complicated with your plots, so this should be fairly straightforward once you get over the admittedly steep learning curve of recipes.

[jarvist]

Thanks for that @wkearn . I've added your instructions to the documentation, and fixed (by commenting out) the obsolete attempt to plot rfsmallalpha.

Hopefully we should be good to go I think!

[lheagy]

👋 Hi @yxqd, would you be willing to take another look at this?

[yxqd]

Thanks @lheagy for the reminder.

Using Juliabox is a nice idea. But I encountered this problem:

And I cannot test the plotting in juliabox either since the calculations failed at cell 4.

I will test plotting from command line soon.

[jarvist]

How frustrating! (...After a bit of investigation...) This is because JuliaBox is still on Optim 0.14 , and it does not pay regard to the REQUIRE file when you clone a package.
I updated the function signature last Friday with 7691566 , so that the package works from the latest Julia 0.6.x with a Pkg.update() (which pulls in the new Optim 0.15 interface).

As a work around, you can add 'Optim' to the 'Yours' list in the 'Packages' menu on the root of JuliaBox. It takes a while to build the package; and then a while to compile after you restart your kernel. This appears to pull in a fresh stable copy.

[yxqd]

Thanks. I tried the command line interface locally, and it does work now after Pkg.update(). Maybe you can add these little problems somewhere in the documentation (maybe a "Known problems" section), or as github issues, so that you can easily refer users to those hints if this comes up again.

The notebook at JuliaBox mostly works now after adding Optim to "registered" packages of JuliaBox. Maybe you want to add this somewhere in your documentation too.

There is still one last error with the JuliaBox notebook:

Please check.

[jarvist]

@yxqd OK Jiao, I've pushed a new version of the ipython notebook without the 'integer division' error (the approximation is no longer calculated as standard).

I've also introduced some text to README.md suggesting that users install the Optim package themselves on Juliabox. Hopefully within a few weeks the standard images on Juliabox will update.

I think this should be sufficient to cover the final checkbox above - "Example usage".

[yxqd]

Just tried it. It seems working now. @lheagy could you please close this ticket?

[lheagy]

This looks great @jarvist!