Closed wd15 closed 1 year ago
@wd15, the new version of the PFHub website is available at https://random-cat-1484.surge.sh
@tkphd, please review after I've merged #1482
@wd15, the new version of the PFHub website is available at https://random-cat-1484.surge.sh
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:15Z ----------------------------------------------------------------
Line #1. benchmark_id = '1d.1'
Wrong ID.
As you know, these notebooks are automatically generated using papermill. The first cell has the original parameters from template.ipynb (which in this case is benchmark_id = '1d.1'
) probably those which I last tested. papermill replaces these with an injected set from a yaml file in the next cell. Basically, the paramters in the first cell should be overwritten by the second cell. I don't really have any control over that.
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:15Z ----------------------------------------------------------------
Line #6. All results for the [{ benchmark_id } benchmark specification](../../benchmarks/benchmark1.ipynb/).
Wrong spec.
Fixed now
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:17Z ----------------------------------------------------------------
Line #5. x=r't',
Use meaningful axis labels with dimensions, e.g.
x = r"simulated time <em>t</em> / [a.u.]",
y = r"simulated free energy #8497; / [J]", # unless F is non-dimensionalized
Ok, should look better now. I used arbitrary units instead of J for the free energy. I'm not sure that the benchmark uses units at all. Ok, the orignal paper, https://doi.org/10.1016/j.commatsci.2016.09.022, doesn't use units. Probably best to leave it as a.u.?
_tkphd commented on 2023-02-15T20:33:11Z_ ----------------------------------------------------------------Right!
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:18Z ----------------------------------------------------------------
Line #7. title="Free Energy v Time",
"Free energy vs. Time" simply reiterates the plot axes -- better to leave the title blank in this case.
Or, if you prefer to leave axis labels as symbols and spell them out in the title, then at least use "vs." for "versus" instead of just "v".
Yup, all the titles are blank now
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:19Z ----------------------------------------------------------------
Line #1. efficiency_plot(benchmark_id)
Recommend "Numerical", "Computational", or "Software" Efficiency -- some phrasing to convey that this is a meta-characteristic, not a measured efficiency in the model (which is undefinable).
Title changed and I've added the text for the asterix.
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:20Z ----------------------------------------------------------------
Timestamp first, newest to oldest.
wd15 commented on 2023-02-15T20:30:33Z ----------------------------------------------------------------
This benchmark hadn't gotten the changes from the master before you reviewed. Should be good now.
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:22Z ----------------------------------------------------------------
Line #1. benchmark_id = '1d.1'
nope
Again, not a problem
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-09T18:23:23Z ----------------------------------------------------------------
Line #6. All results for the [{ benchmark_id } benchmark specification](../../benchmarks/benchmark1.ipynb/).
wrong spec
Fixed
As you know, these notebooks are automatically generated using papermill. The first cell has the original parameters from template.ipynb (which in this case is benchmark_id = '1d.1'
) probably those which I last tested. papermill replaces these with an injected set from a yaml file in the next cell. Basically, the paramters in the first cell should be overwritten by the second cell. I don't really have any control over that.
View entire conversation on ReviewNB
@wd15, the new version of the PFHub website is available at https://random-cat-1484.surge.sh
Right ... I guess I'm expecting to see things like {{ benchmark_version }}
in template.ipynb, which then get converted to meaningful strings in the output. It looks like this PR is committing papermill-generated notebooks that contain wrong information, which feels wrong.
Ok, should look better now. I used arbitrary units instead of J for the free energy. I'm not sure that the benchmark uses units at all. Ok, the orignal paper, https://doi.org/10.1016/j.commatsci.2016.09.022, doesn't use units. Probably best to leave it as a.u.?
View entire conversation on ReviewNB
This benchmark hadn't gotten the changes from the master before you reviewed. Should be good now.
View entire conversation on ReviewNB
I'm not sure why they've chosen this method. I guess it's so you can run the template notebook as a regular notebook without requiring the injection of parameters. It would be better if the original parameters were commented out.
Oh right, that makes total sense. It'd be harder to verify that the template itself works, if the inputs were all squirrely.
@tkphd, all looks good to me and I think I've addressed everything you noticed. Please review again.
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-15T22:49:39Z ----------------------------------------------------------------
The previous link went to the benchmark spec; the new link is to this notebook itself, which is a little at odds with the link text. Which is it supposed to be?
wd15 commented on 2023-02-16T00:12:04Z ----------------------------------------------------------------
../../benchmarks/benchmark1a.1.ipynb
, for example, drops straight to ../../benchmarks/benchmark1.ipynb,
which is the spec. You can check that it works here, https://random-cat-1484.surge.sh/results/benchmark1a.1.ipynb/
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-15T22:49:40Z ----------------------------------------------------------------
Better! The code ...␣<xyz>␣ ␣<uvw>
produces a triple-space, maybe preferable to write ...␣<xyz> <uvw>
wd15 commented on 2023-02-16T00:42:46Z ----------------------------------------------------------------
Should be better now
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-15T22:49:41Z ----------------------------------------------------------------
Same as the BM1 notebooks: this ought to link to the spec, right? Rather than itself, the results page?
wd15 commented on 2023-02-16T00:43:07Z ----------------------------------------------------------------
links are good
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-15T22:49:43Z ----------------------------------------------------------------
Probably want to substitute the major benchmark number here, not the sub-part and version, so it links to the spec and not itself.
wd15 commented on 2023-02-16T00:43:22Z ----------------------------------------------------------------
no, it works as is
View / edit / reply to this conversation on ReviewNB
tkphd commented on 2023-02-15T22:49:44Z ----------------------------------------------------------------
See ␣ ␣
comments on the outputs above.
wd15 commented on 2023-02-16T00:43:31Z ----------------------------------------------------------------
done
../../benchmarks/benchmark1a.1.ipynb
, for example, drops straight to ../../benchmarks/benchmark1.ipynb,
which is the spec. You can check that it works [here](https://random-cat-1484.surge.sh/results/benchmark1a.1.ipynb/).
View entire conversation on ReviewNB
@wd15, the new version of the PFHub website is available at https://random-cat-1484.surge.sh
@tkphd, hopefully good now. I'll reset the check for you to go again. Phew!
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB