Integrate part of spot check - simulated and historical data prep and plot by state

[jenhagg]

Purpose - Move logic from the spot check notebook into the code base.
What it does - Add functions to prepare data for analysis and one of the plots, and refactored code where possible for readability and/or using built in pandas methods (hopefully idiomatically). There are some unit tests for the data manipulation logic but could probably add more.

The main additions, in terms of what I'd expect to be used by the notebook, are the following:

# simulated generation based on a scenario
sum_generation_by_state(scenario_info):

# historical generation, based on a csv or excel file loaded by the user into hist_gen_raw
summarize_hist_gen(hist_gen_raw, all_resources):

# plot given state
plot_generation_sim_vs_hist(sim_gen, hist_gen, s_info, state, showmax=True):

Time to review - 30 mins (rough estimate). For testing I compared results of the original notebook with these functions, to spot check (pun intended) that we get the same results numerically, visually, etc. The unit tests cover some basic assumptions as well but I think it's a good point to get user feedback, or try it on other scenarios before adding a bunch more test coverage. I'm pretty sure the docstrings can be improved but wasn't sure what best practices are so just left for code review.

Questions - Would it be helpful to pair review? I can demo my notebook used for testing if that's useful. And how do we update the notebook script once we want to start using these?

[BainanXia]

Not sure whether this is necessary here, given it simply calls the get_generation function via an instance of ScenarioInfo class, which can be put into a one-liner.

[BainanXia]

I understand the reason of breaking this up into parts is for simplicity of testing. However, maybe it is worth creating a Mock ScenarioInfo object or improve the Mock Scenario object with time series attributes such that it can be loaded into a ScenarioInfo object, since ScenarioInfo is widely used in the spot check integration script, which will make the tests in the future much easier once we have a mock for that.

[jenhagg]

I was pondering the same thing. Think you're right about simplifying future code/testing, so I'll look into setting up a mock implementation. Leaning towards mocking ScenarioInfo itself, so the tests for spot check logic are totally independent of ScenarioInfo implementation, which should be tested separately against mock Scenario or State instances. At least, that's my current frame of mind, we'll see how it goes :)

[BainanXia]

Thanks for handling this ugly setup :)

[danielolsen]

Is this tuple 'states that are split across interconnections'? If so, do you think this would fit better as a named global variable instead of hardcoded within this variable?

[jenhagg]

I'm not sure, but some examples from the raw data are "Montana Western", "Texas North Central", "Texas Far West" etc. Is that consistent with your description?

[danielolsen]

Yeah. The TAMU network mostly follows state lines when approximating the borders between interconnections (even though small bits of some plains states stretch over the lines), I believe Montana, Texas, and New Mexico are the only exceptions.

[BainanXia]

Once we start to finalize this PR, we should come up with better names of these intermediate functions together with other parameter documentations.

[BainanXia]

Organizing these plot parameters in a class is clean which is something in my mind for a while. However, setting up these parameters with hard coded constants is tricky given they might not fit with all the plots including the future ones we may have, specifically here, different bar plots may have adaptive bar width according the total number of bars within a plot. Let's think about a better way to handle general cases later.

[jenhagg]

Sounds good, I figured we'd need to do more here but left it as kind of a reminder.

[BainanXia]

Not sure whether we need functions like this in the codebase. I think having a simple loop to call a function with parameters is acceptable in the user's script.

[BainanXia]

What kind of warnings we are trying to simplify here?

[BainanXia]

Ah, I see, it's from get_capacity in ScenarioInfo object. Basically, when we are trying to get capacities of a specific type of generation which does not exist in certain area, it returns 0 with warnings.

[jenhagg]

Ah, I didn't actually know that. We have suppression in the notebook, so I just switched it to use the context manager since that seemed right.

[BainanXia]

When I was implementing these functions, I simply wrote down the code that appeared on the top of my head. Not sure whether there are cleaner ways to set up these parameters when prepare a plot. If it looks bad to you, free feel to refactor :)

[jenhagg]

Good to know :) I'm pretty amateur at matplotlib so played it safe for now, but I'll come back to it and see what we can do.

[BainanXia]

Same here, I simply found an example from StackoverFlow of labeling the bars in a bar chart and used it here.

[BainanXia]

I've heard of it but never used it before. I presume monkeypatch is defined in pytest.fixture, which makes it easy to mock function returns in a mock object.

[BainanXia]

I've gone through this again together with the previous PR on MockScenarioInfo, https://github.com/Breakthrough-Energy/PowerSimData/pull/227/files. I think the testing part is much cleaner with MockScenarioInfo. Thanks for the effort. I will approve it once our file system is ready and I could check it with some integration tests. In the meantime, it would be great if we can get a second eye on this given it is the first PR on moving stuff in spot check into code base, which sets an example of merging plot features into PostREISE. We will refactor AnalyzePG in the same fashion.

[danielolsen]

Could we correct the column names in the input CSV and avoid this dictionary?

[jenhagg]

Yeah that'd be cleaner, not sure how the csv columns are generated though, or if it's part of some process. Probably doesn't hurt to have this kind of mapping but could move it somewhere more intentional if there are other usages.

[danielolsen]

Good point, I don't remember where they come from.

[BainanXia]

I brought this up long time ago to @danlivengood who creates these input CSVs. At that time the argument was that "names on the left (keys in colname_map) make more sense for human reading, whereas names on the right (values in colname_map) make more sense for computer reading (what we defined in the grid DataFrame)."

[danielolsen]

Looks good. The spot check script quickly grew into a beast, I'm sorry that you got tasked with cleaning up our mess.

[jenhagg]

I've seen much worse of a mess, glad to do it (it's also informative). One thing I noticed though is the plot fails on Texas since we have "Texas" in the index for both state and interconnect. I'll look at a fix for that before merging.

[BainanXia]

One thing I noticed though is the plot fails on Texas since we have "Texas" in the index for both state and interconnect. I'll look at a fix for that before merging.

Good catch, previously it's fine since we were summarizing simulated results via ScenarioInfo and function area_to_loadzone took care of the confusion: Texas is treated as a state by default, unless the area_type is specified as 'interconnect'.

[BainanXia]

This is nice. Thanks.