Doc: Add documentation for pyreisejl usage #88

ahurli · 2020-12-07T19:21:46Z

Purpose

Document how to use

What is the code doing

The README now includes documentation around how to use pyreisejl (i.e. all the different possible flags) as well as some additional detail about what additional functions these python scripts provide.

It assumes that the user already has the proper setup for Julia, Gurobi, and python.

Where to look

This is all in the README. No code has been touched.

Time estimate

10min

README.md

rouille · 2020-12-09T22:50:50Z

README.md

@@ -46,6 +46,166 @@ REISE.run_scenario(;
    inputfolder=pwd(), num_segments=3)
 ```

+## Usage (Python)
+
+The python scripts included in `pyreise` perform some additional input validation for the Julia engine before running the simulation and extract data from  the resulting `.mat` files to `.pkl` files.


pyreise --> pyreisejl

README.md

rouille · 2020-12-09T23:43:04Z

README.md

+```
+  -h, --help            show this help message and exit
+```
+Instead of calling `add PACKAGE`, it is also possible to call `dev PACKAGE`, which will always import the latest version of the code on your local machine. See the documentation for the Julia package manager for more information: https://julialang.github.io/Pkg.jl/v1/.


I am not sure the above paragraph should be in this Running Simulation section.

Thank you! I don't think so either.

ToddG

This is great but with my newbie to REISE.jl lense, I feel this document needs:

dependencies section
high level walk through of the user interactions (scripts, etc.)

I definitely feel that since we are tightly coupled with Gurobi, there is going to be a significant barrier to adoption... case in point. Normally as part of a PR I would clone this repo, install the deps, run through the sample commands, ...basically try it out and kick the tires. But w/o said license, I'm just looking on from the sidelines.

ToddG · 2020-12-10T18:16:30Z

README.md

@@ -16,7 +16,7 @@ pkg> activate .
 Another way is to install the package using the list of dependencies specified in the `Project.toml` file, which will pull the most recent allowed version of the dependencies. Currently, this package is known to be compatible with JuMP v0.21.3; this is specified in the `Project.toml` file, but there may be other packages for which the latest version does not maintain backward-compatibility.

 This package is not registered. Therefore, it must be added to a Julia environment either directly from GitHub:


For line 6 above:

Note: If Gurobi.jl is not already installed

I don't think the If should be capitalized here.

For line 4 above:

It seems that the readme is missing a top level dependencies section? I'd expect to see that at the top, so I know where to go to get all the deps etc.

Makes sense! I'll add it in.

ToddG · 2020-12-10T18:18:01Z

README.md

@@ -30,7 +30,7 @@ Instead of calling `add PACKAGE`, it is also possible to call `dev PACKAGE`, whi
 The dependencies of the python scripts contained in `pyreisejl/` are not automatically installed. See `requirements.txt` for details.


Why not add the command here to install the requirements? It's just simple

pip install -r requirements.txt

Or something similar...

ToddG · 2020-12-10T18:19:51Z

README.md

@@ -30,7 +30,7 @@ Instead of calling `add PACKAGE`, it is also possible to call `dev PACKAGE`, whi
 The dependencies of the python scripts contained in `pyreisejl/` are not automatically installed. See `requirements.txt` for details.


-## Usage
+## Usage (Julia)
 Installation registers a package named `REISE`. Following Julia naming conventions, the `.jl` is dropped. The package can be imported using: `import REISE` to call `REISE.run_scenario()`, or `using REISE` to call `run_scenario()`.


Why not include the import statement in the code snippet in line 38 below. This way a user can see a full working example and just copy and paste.

Makes sense! I'm planning on doing a followup PR to revise the Julia/Gurobi bits of the README, so I'll use this feedback for that PR.

ToddG · 2020-12-10T18:21:37Z

README.md

@@ -30,7 +30,7 @@ Instead of calling `add PACKAGE`, it is also possible to call `dev PACKAGE`, whi
 The dependencies of the python scripts contained in `pyreisejl/` are not automatically installed. See `requirements.txt` for details.


-## Usage
+## Usage (Julia)
 Installation registers a package named `REISE`. Following Julia naming conventions, the `.jl` is dropped. The package can be imported using: `import REISE` to call `REISE.run_scenario()`, or `using REISE` to call `run_scenario()`.

 To run a scenario which starts at the `1`st hour of the year, runs in `3` intervals of `24` hours each, loading input data from your present working directory (`pwd()`) and depositing results in the folder `output`, call:


Lines 38-46 below... what are the expected generated output artifacts? What does the julia output log look like? This is where we can help guarantee users are seeing expected behaviour before they tackle more complicated use cases.

Agreed! But as commented above, I'll use this feedback for the followup PR.

ToddG · 2020-12-10T18:23:18Z

README.md

@@ -46,34 +46,195 @@ REISE.run_scenario(;
    inputfolder=pwd(), num_segments=3)
 ```

+## Usage (Python)
+
+The python scripts included in `pyreisejl` perform some additional input validation for the Julia engine before running the simulation and extract data from  the resulting `.mat` files to `.pkl` files.


I'd suggest a line length limit not to exceed 80 characters. It seems this document is not being linted with that sort of check...

What linter would you use for Markdown documents @ToddG?

ToddG · 2020-12-10T18:23:43Z

README.md

+
+The python scripts included in `pyreisejl` perform some additional input validation for the Julia engine before running the simulation and extract data from  the resulting `.mat` files to `.pkl` files.
+
+For example, a simulation with automatic extraction can be run as follows:


What is automatic extraction?

Good call. I think I might just take that out here because it's not a required input, and it's addressed later on.

ToddG · 2020-12-10T18:29:24Z

README.md

+
+To run the `REISE.jl` simulation from python, run `call.py` with the following required options:
+```bash
+  -s, --start-date START_DATE


What's the story w/respect to Time Zones? This start/end date format does not contain Time Zone Designators (https://en.wikipedia.org/wiki/ISO_8601). Is it assumed to be UTC? If so perhaps that should be stated...

We assume that the time zones of the start date & end date match the time zone of the demand/hydro/solar/wind profiles (UTC).

ToddG · 2020-12-10T18:31:38Z

README.md

+specify the execute directory:
+```bash
+  -x EXECUTE_DIR, --execute-dir EXECUTE_DIR
+                        The directory to store the results. This is optional


what happens if either the output directory (execute directory...boy that's a wonky name) exist already? Are the outputs overwritten or is the computation aborted?

Currently, outputs are overwritten.

ToddG · 2020-12-10T18:32:22Z

README.md

+simulation run:
+```bash
+  -t THREADS, --threads THREADS
+                        The number of threads to run the simulation with.


a) indentation

b) what happens if you specify zero threads? what's the max number? what if you exceed the max number?

Good question. Zero threads defaults to auto, though the invocation of this script from powersimdata requires that it be greater than 0. Negative values do error out eventually, and that's a good call-out for validation that should be added, but I think that might be outside the scope of this PR.

The max number seems to be a little trickier, since I think that gets passed to Gurobi, so that changes depending on what kind of Gurobi license you're using. If you exceed the max number, though, it seems like it'll still run, but just give you a warning:

Thread count: 8 physical cores, 16 logical processors, using up to 200 threads Warning: Thread count (200) is larger than processor count (16) Reduce the value of the Threads parameter to improve performance

ToddG · 2020-12-10T18:34:42Z

README.md

+
+# Extracting Simulation Results
+
+The script `extract_data.py` extracts the following Pandas DataFrames from the


How many scripts are there? What's the high level user interaction with this system? It would be nice to have a section at the beginning that outlines how the user interacts with this system, what the different scripts are, etc. Just a high level sort of lay-of-the-land. Then you can dive into each of these commands/scripts/interactions in the guts of the readme. Right now I'm left with the feeling of wading through a mish-mash of scripts.

There are only two different python scripts which can be combined (i.e. have the second run automatically after the first, decoupled for memory constraint reasons). I'll add some additional notes at the top of the Usage (Python) section, but I definitely think a high-level blurb at the very top would be useful.

danielolsen · 2020-12-10T19:36:57Z

@ToddG we have a shared Gurobi cloud license that you can use for testing, stored on the Compute server. Do you have access?

danielolsen · 2020-12-14T21:33:52Z

The commit history will need to be cleaned up here. If I set up the branch rule properly, even with an approval on the PR Github should not allow such a 'non-linear' history to be merged.

rouille · 2020-12-14T22:39:36Z

The commit history will need to be cleaned up here. If I set up the branch rule properly, even with an approval on the PR Github should not allow such a 'non-linear' history to be merged.

I agree. Since it is only about documentation, I would do an interactive rebase where all the commits are combined in one, i.e., all the commits under the first one are set to fix and reword the the only commit left to something like docs: write documentation for pyreisejl.

README.md

danielolsen · 2020-12-15T00:18:06Z

README.md

@@ -46,6 +64,187 @@ REISE.run_scenario(;
    inputfolder=pwd(), num_segments=3)
 ```

+## Usage (Python)
+
+<<<<<<< HEAD


Something has gone wrong in this rebase/merge.

Also, I don't see the changes you previously made in call.py anymore

Yeah, I was struggling with the merge conflicts in the README. I'll need to go through this a couple more times/more thoroughly to get it updated.

danielolsen · 2020-12-15T00:29:17Z

README.md

+[Gurobi Installation Guide]: https://www.gurobi.com/documentation/quickstart.html
+[Julia]: https://julialang.org/
+[Download Julia]: https://julialang.org/downloads/
+[Zenodo]: https://zenodo.org/record/3905429


Please update this link to https://zenodo.org/record/3530898. This is the URL that will always resolve to the latest version of our dataset on Zenodo (which happens to be 3905429 at the moment but will change as we upload new versions).

danielolsen · 2020-12-15T00:34:44Z

README.md

+  -f [FREQUENCY], --frequency [FREQUENCY]
+                        The frequency of data points in the original profile
+			csvs. This is optional and defaults to an hour.


What type of input are we expecting here? E.g. if I want to specify a 4-hour frequency, do I need to add -f 4 or -f 4H (a guess here, based on creating scenarios with PowerSimData).

Good question. It's a Pandas frequency string as defined for pandas.date_range which I'm not entirely clear the extent of, but eventually this input is something I would want to save in a Manifest or something similar (i.e. inspired by @ToddG 's work for CERF) since it's automatically calculated in call.py.

danielolsen · 2020-12-15T04:35:31Z

pyreisejl/utility/extract_data.py

+        help="The frequency of data points in the original profile csvs as a"
+        "Pandas frequency string ."


Spacing is off here:

-f [FREQUENCY], --frequency [FREQUENCY] The frequency of data points in the original profile csvs as aPandas frequency string .This is optional and defaults to an hour.

danielolsen

Thanks, this looks good.

ahurli requested review from ToddG, danielolsen, rouille and jenhagg December 7, 2020 19:21

ahurli self-assigned this Dec 7, 2020

jenhagg reviewed Dec 8, 2020

View reviewed changes

README.md Show resolved Hide resolved