Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate spot check code for plotting multiple scenarios #44

Merged
merged 5 commits into from
Nov 22, 2019

Conversation

merrielle
Copy link
Collaborator

@merrielle merrielle commented Nov 8, 2019

This pr integrates our spot check code into PostREISE. I decided I wanted to avoid messing with analyze_pg so as to not step on any toes. We can refactor everything together in the future. I intended to use this code structure as a base for future plotting code. Each graph type has its own file with only one (or two, for bar and hbar graphs) public facing functions. There is a common file for helper functions where the plotting code overlaps.

For the record I am not a fan of posting huge reviews like this -- I think it's better for everyone's sanity to break tasks into small bits and keep PRs around 200 lines if possible. So I appreciate your work in reviewing this. :)

@merrielle merrielle requested review from dmuldrew and rouille November 8, 2019 17:54
@kasparm kasparm self-requested a review November 8, 2019 19:06
@kasparm
Copy link
Contributor

kasparm commented Nov 8, 2019

Thanks for the work. Could you please read through our guidelines(https://github.com/intvenlab/REISE/wiki/Software-Development-Guidelines) and follow the conventions please. Especially how to document the code and the format of the commit messages.

@merrielle merrielle changed the title Spot tech debt Feat: Integrate spot check code with feature to plot multiple scenarios Nov 9, 2019
@merrielle merrielle changed the title Feat: Integrate spot check code with feature to plot multiple scenarios Feat: Integrate spot check code for plotting multiple scenarios Nov 9, 2019
@kasparm
Copy link
Contributor

kasparm commented Nov 11, 2019

@merrielle you can rebase to develop now.

@merrielle merrielle force-pushed the spot_tech_debt branch 3 times, most recently from 6167b77 to 290d9bc Compare November 13, 2019 19:35
@kasparm kasparm added this to the Sprint 1 milestone Nov 14, 2019
@rouille
Copy link
Collaborator

rouille commented Nov 14, 2019

I did the following:

from postreise.plot.multi.plot_bar import plot_bar

plot_bar('Western', ['87'])

and I got a plot for all the load zones in the Western interconnection. I noticed that the legend/title are overlapping. This is shown below for Idaho.
download

I understand that the routines are built to compare multiple scenarios. Should we:

  • raise an error if the list of scenario ids has only one element.
  • or we want to have a bar plot for a single scenario. In that case, we need to rearrange the titles and print only one scenario name at the bottom.

@rouille
Copy link
Collaborator

rouille commented Nov 15, 2019

I noticed some style issues (PEP8 and others):

  • Every line (code, comment, docstring) should be < 80 characters.
  • Unused import statement:
    numpy in plot_bar
    pandas in plot_helpers
    ALL_RESOURCE_TYPES in plot_pie
    matplotlib.pyplot and numpy in plot_shortfall
  • Issue with docstring in:
    plot_hbar (no parameter interconnect)
    unit_conversion (return statement needs to be located after the docstring)
    _construct_pie_visuals (no parameter ax_data)
  • Also, I think we want to populate __all__ in the init.py

@rouille
Copy link
Collaborator

rouille commented Nov 15, 2019

Do you think it would be possible to have a keyword in the routines to set the scenario nane? When not None it will not fetch the horibble scenario name. To illustrate, scenario #87 has name: WesternBase_2016_noHVDC_Final_2019Sep. I would say it is better to have something different in the titles or legends.

@rouille
Copy link
Collaborator

rouille commented Nov 15, 2019

It is possible to pass a custom_data dictionary in plot_bar and plot_pie. I believe this is used to produce the kind of plot that compares our generation levels wiith NREL's. I was wondering if we should not have a data folder in postreise/plot/multi where the NREL (and other exterior data are stored) and write a helper function that would read the data and produce a custom_data dictionary. I believe this would be useful to anybody who want to make this plot.

@rouille
Copy link
Collaborator

rouille commented Nov 15, 2019

Regarding the tests, is there any reason why you don't use the unittest unit testing framework?

@rouille
Copy link
Collaborator

rouille commented Nov 20, 2019

As mentioned during the meeting this morning, I think it would be nice that the user can select the start and end dates for the chart. In the past we have been looking at generation level for the summer, winter or even a single month.

@merrielle
Copy link
Collaborator Author

merrielle commented Nov 20, 2019

Re: plotting a single scenario
I think that's functionality we should definitely add, but maybe in a future PR. For now I'll add an error if there's only one scenario.

Re: style issues
Will fix! Also, is there a reason we add __all__ to the init file? I know you don't have to but maybe there's something I'm missing.

Re: scenario name
Yeah... I was thinking about doing that and decided to leave it alone for now. I can add it since it seems important

Re: NREL custom data
That's a great point and I think it would be useful. I'd want to think more about where that data is stored. After this PR is done I'll make a new PR to add it.

Re: tests
Software Dan and I think pytest is better because you need to write less boilerplate code in order to write tests. I think by this point we've told everyone via the slack and in person. Luckily we don't need to change any of the old unittest tests because pytest can run those as-is.

@kasparm
Copy link
Contributor

kasparm commented Nov 21, 2019

@dmuldrew please update the testing guideline to reflect the fact that we use pytest.

@merrielle
Copy link
Collaborator Author

@rouille I've finished all the fixes you requested :)

Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thanks a lot, it is fantastic.

@kasparm
Copy link
Contributor

kasparm commented Nov 21, 2019

Do we have any demo or readme for this?
What is the plan? This is code that researcher going to us.

Merrielle Ondreicka added 5 commits November 21, 2019 17:22
rebase: Add all analyze code in one file, refactor code for better documentation and testability

rebase: Break everything into seperate files by graph type. Add mocks and tests
fix: Small tweaks before PR

refactor: Change test dir to tests. Make small changes to scenario import and tweaks to visuals

fix: Deepcopy mock objects to prevent test contamination

fix: Remove mock objects from global namespace

style: Follow pycodestyle and pep8 guidelines
refactor: split shortfall inputs and plot helpers

fix: keep bar plots from overlapping, fix target line position for shortfall
Copy link
Contributor

@kasparm kasparm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.
There are a few things we need to synchronize when we refactor. The part of the constants are covered in the gird object.
Also please add the readme to your todo list. For now we can merge this.

@merrielle merrielle merged commit 3aab4e7 into develop Nov 22, 2019
@merrielle merrielle deleted the spot_tech_debt branch November 22, 2019 19:55
@ahurli ahurli mentioned this pull request Mar 16, 2021
@rouille rouille changed the title Feat: Integrate spot check code for plotting multiple scenarios Integrate spot check code for plotting multiple scenarios Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants