Documentation imp 2

[Rlamboll]

Pull request

Please confirm that this pull request has done the following:

• Tests added
• Documentation added (where applicable)
• Example added (either to an existing notebook or as a new notebook, where applicable)
• Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Added feature which does something
- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Fixed bug identified in (`#YY <https://github.com/znicholls/silicone/issues/YY>`_)

[codecov]

Codecov Report

Merging #49 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   98.22%   98.21%   -0.01%     
==========================================
  Files          18       18              
  Lines         789      785       -4     
  Branches      166      165       -1     
==========================================
- Hits          775      771       -4     
  Misses          8        8              
  Partials        6        6              

Impacted Files	Coverage Δ
...cone/multiple_infillers/infill_composite_values.py	100.00% <ø> (ø)
src/silicone/utils.py	100.00% <ø> (ø)
src/silicone/database_crunchers/constant_ratio.py	100.00% <100.00%> (ø)
...one/database_crunchers/quantile_rolling_windows.py	100.00% <100.00%> (ø)
src/silicone/database_crunchers/time_dep_ratio.py	100.00% <100.00%> (ø)
...illers/decompose_collection_with_time_dep_ratio.py	95.74% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a398fb...bd3782e. Read the comment docs.

[znicholls]

@Rlamboll do you only want my review?

[Rlamboll]

@znicholls there have been a lot of changes recently so this would be a good opportunity for you to look them over. This is the last big PR planned. @jkikstra can also do it but has been somewhat overwhelmed recently

[znicholls]

Ok cool I’ll chip away at it then. Given how big it is it might take me a few days/weeks.

As a side note, multiple smaller PRs are much faster than one big one because the complexity scales exponentially with the number of changes. I can understand why this one touches lots of things though.

[Rlamboll]

OK, the majority of these files were only changed by me running black. There's a semi-functional change in constant_ratio.py (affects how it treats nans) and the notebooks are completely rewritten but it's not as bad as it looks. It might actually be better if @jkikstra reviews the notebooks on the basis that we want to know that people who weren't involved in coding it can understand it.

[jkikstra]

@znicholls @Rlamboll okay I will review the notebooks in the coming days

[jkikstra]

Before this will be published, it would be good to provide a reference for the data, e.g.

Daniel Huppmann, Elmar Kriegler, Volker Krey, Keywan Riahi, Joeri Rogelj, Katherine Calvin, Florian Humpenoeder, Alexander Popp, Steven K. Rose, John Weyant, Nico Bauer, Christoph Bertram, Valentina Bosetti, Jonathan Doelman, Laurent Drouet, Johannes Emmerling, Stefan Frank, Shinichiro Fujimori, David Gernaat, Arnulf Grubler, Celine Guivarch, Martin Haigh, Christian Holz, Gokul Iyer, Etsushi Kato, Kimon Keramidas, Alban Kitous, Florian Leblanc, Jing-Yu Liu, Konstantin Löffler, Gunnar Luderer, Adriana Marcucci, David McCollum, Silvana Mima, Ronald D. Sands, Fuminori Sano, Jessica Strefler, Junichi Tsutsui, Detlef Van Vuuren, Zoi Vrontisi, Marshall Wise, and Runsen Zhang.
IAMC 1.5°C Scenario Explorer and Data hosted by IIASA.
Integrated Assessment Modeling Consortium & International Institute for Applied Systems Analysis, 2019.
doi: 10.5281/zenodo.3363345 | url: data.ene.iiasa.ac.at/iamc-1.5c-explorer

[Rlamboll]

OK, I've made it shorter but there's a citation

[jkikstra]

"loaded as a"

[Rlamboll]

yep

[jkikstra]

For the 3rd sentence, 2nd part it is clear what is meant, but the sentence is not correct

[Rlamboll]

rephrased

[jkikstra]

It would probably be good to also print (a snapshot) of the reduced database here, e.g. by using .timeseries(), or otherwise .scenarios(), .head(), .variables(), or .models().

[jkikstra]

point out clearly that this relationship can later be used for providing a leader and follower, and how this will be used in the rest of the tutorial (do this here already, and not later, to make the use of this function more clear), referring back to the derive_relationship docstring that was printed earlier.

[Rlamboll]

ok, more added.

[jkikstra]

In general:

• great job on the notebooks! I've learned a lot, which is the goal of such notebooks :)
• I can imagine that for a lay-user/non-modeler, some of the texts and code are still slightly too complex, but I'm not sure if we want to write for such an audience anyway?
• for the introductory notebook, I've provided some minor comments in-code

Commenting inline for More_crunchers.ipynb is not allowed, so I'll put them here:

• very nice introduction on pre-filtering data! Would it maybe be good to just split this up and have it as a separate notebook, instead of in this one?
• typos: 'very will' -> 'very well' ; 'this technique is extracts' -> 'this technique extracts'
• for the case where negative values dominate, it would be good to show a plot that shows those negative values
• also point out in the descriptive text that the average will be used with more than one value (for the interpolation method)

[jkikstra]

There is a part of this text that seems missing, and the formatting seems off (you can put "Recovering the data you put in") in the next cell already.

[Rlamboll]

ok

[jkikstra]

Nice, good to see some pointers on how these infilling options are best used.

[Rlamboll]

OK, I've responded to all of @jkikstra 's points other than splitting the notebook up. Is that something that would be helpful? It's quite a short section, but it is logically separate. @znicholls was suggesting that each cruncher has a separate notebook, don't know if this is just annoying to look through though.

[jkikstra]

In general I prefer shorter notebooks with one clear topic over longer ones, mostly because of the added benefit that it is easier the find what you're looking for when you're browsing the directory.

[Rlamboll]

OK, I'll do a separate PR where I split the notebooks up into smaller, numbered chapters.

[jkikstra]

Concerning the notebooks this looks good!

[znicholls]

Looks good. Let's rip out the installation stuff and address in #53.

Nice work @Rlamboll !!

[znicholls]

Let's skype about this. Best practice is to release like is done for scmdata.

That means users can then install simply by doing pip install silicone or conda install silicone. If they want a specific version, they can specify e.g. pip install silicone==0.2.1. With releases on pypi (pip) and conda, we don't have to worry about a 'release version' and a 'development version'. I would suggest we simply do away with the znicholls/silicone (when you're ready) and move it all to GranthamImperial.

[Rlamboll]

This seems like a good idea, but to be done in a separate pull request. Currently what's written there is true, so I'm inclined to stick with that and update it later.