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

Clean notebooks #327

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Clean notebooks #327

merged 3 commits into from
Nov 30, 2020

Conversation

rouille
Copy link
Collaborator

@rouille rouille commented Nov 2, 2020

Pull Request Etiquette doc

Purpose

Clean notebooks

What the code is doing

No code

Where to look

  • The powersimdata/scenario/demo/tutorial.ipynb notebook has been deleted
  • powersimdata/scenario/demo/scenario_and_grid_cheatsheet.ipynb: I have rerun the notebooks using a benchmark scenario, add a title, etc.
  • powersimdata/input/demo/branch_device_type.ipynb: Same as above. I have also fixed some warnings. This notebook uses a package, networkx, which is not in our requirements.txt/Pipfile. Should we add it to the optional-requirements.txt? What do you think @danielolsen and @jon-hagg

Time estimate

20min.

@rouille rouille added the documentation Documentation related to package label Nov 2, 2020
@rouille rouille added this to the VOTE milestone Nov 2, 2020
@danielolsen
Copy link
Contributor

danielolsen commented Nov 2, 2020

I can see how these notebooks were useful for us, but I'm not sure it's worth maintaining them as notebooks going forward vs. pulling the main conclusions out and recording that information in markdown-format in the main README (for scenario_and_grid_cheatsheet.ipynb) or in a TAMU-specific README (for branch_device_type.ipynb).

@rouille
Copy link
Collaborator Author

rouille commented Nov 2, 2020

I can see how these notebooks were useful for us, but I'm not sure it's worth maintaining them as notebooks going forward vs. pulling the main conclusions out and recording that information in markdown-format in the main README (for scenario_and_grid_cheatsheet.ipynb) or in a TAMU-specific README (for branch_device_type.ipynb).

I agree 100%. I believe that the README already encloses all the information located in scenario_and_grid_cheatsheet.ipynb. What would you add to the README? Regarding branch_device_type.ipynb, should I move it in powersimdata/network/usa_tamu/?

@BainanXia
Copy link
Collaborator

I agree with @danielolsen on the point that we might want to maintain these introductions in a lighter way instead of maintaining a notebook in the codebase, which is not convenient to read from general perspective. These notebooks are useful when we are exploring the tamu model ourselves and now we are presenting it others, thus probably we could provide a table that defines the columns of dataframes as Matpower did in the user manual and let the users explore themselves by loading the corresponding object instance into their workspace?

@BainanXia
Copy link
Collaborator

@rouille If we are confident that the current README has covered all the info we present in scenario_and_grid_cheatsheet.ipynb, then we could remove it. Regarding the other notebook, it is purely tamu_model dependent info, I think it is fair enough to move it in powersimdata/network/usa_tamu/ if we don't want to extract those info into other forms (such as tables+explanation in README) at this point.

@rouille
Copy link
Collaborator Author

rouille commented Nov 3, 2020

Won't all network models have the same structure so they can be represented by a Grid object? In the affirmative, I think it would make sense to have a README in powersimdata/input/ where the Grid class is located where we list the different fields of the data frames of the attributes composing the Grid class (plant, gencost, etc.).

@danielolsen
Copy link
Contributor

The branch_device_type.ipynb notebook seems to be specifically investigating the branch types of the TAMU network. Our next network may not even have TransformerWinding elements.

The Grid object should ideally have all of the same attributes, even for new network models, but we may not necessarily have all of the same columns in the tables themselves. E.g., should we generate data for the new models that we don't use (ramp_10, apf, etc.) just to ignore it in our simulation engine? How long do we want to support all the MATPOWER columns?

At some point we may also need new information to run Scenarios, such as unit commitment details for more detailed production cost model runs, or investment cost details for capacity expansion model runs (if we will still be preparing/launching those using PowerSimData).

These aren't things we need to consider right now, but definitely things to keep in the back of our minds.

@rouille
Copy link
Collaborator Author

rouille commented Nov 3, 2020

I guess I will move the branch_device_type notebook in powersimdata/network/usa_tamu/ then. I agree that a large number of fields in the data frames of the Grid class are now useless that we move away from MATPOWER. We should think about refactoring the Grid class to stop generating some case.mat and in turn grid.mat with information we do not need. I will have an additional opportunity to modify the grid.mat!

@jenhagg
Copy link
Collaborator

jenhagg commented Nov 3, 2020

I'm wondering if it makes sense to have a single optional-requirements.txt since users might need various subsets of that on a case by case basis. One thing we could do is have the notebook itself install the package. That said, there is only one optional package at the moment, so don't think it will hurt to add one more.

@rouille rouille changed the title README and notebooks Clean notebooks Nov 9, 2020
@rouille rouille force-pushed the ben/notebook branch 3 times, most recently from fed3b67 to 458bb23 Compare November 14, 2020 04:28
@rouille rouille force-pushed the ben/notebook branch 2 times, most recently from 3b172a8 to a992b03 Compare November 17, 2020 21:20
@kasparm
Copy link
Contributor

kasparm commented Nov 20, 2020

@rouille What is the status on this?

@rouille
Copy link
Collaborator Author

rouille commented Nov 20, 2020

@rouille What is the status on this?

The notebooks are cleaned/refactored. @BainanXia, @danielolsen and I did not make a decision on where they should go (move in other folder or Dropbox)

@danielolsen
Copy link
Contributor

I vote that we save branch_device_type.ipynb to our dropbox and remove it from our repo. I think scenario_and_grid_cheatsheet.ipynb should stay, but we should get serious about doing automated checks in our CI process to make sure that when we change our codebase, we also detect when this breaks notebooks that are in the repo, so that we can either make sure to update them or decide that the updating process is not worth it and remove them.

@rouille
Copy link
Collaborator Author

rouille commented Nov 28, 2020

I vote that we save branch_device_type.ipynb to our dropbox and remove it from our repo. I think scenario_and_grid_cheatsheet.ipynb should stay, but we should get serious about doing automated checks in our CI process to make sure that when we change our codebase, we also detect when this breaks notebooks that are in the repo, so that we can either make sure to update them or decide that the updating process is not worth it and remove them.

I am not super familiar with the Dropbox structure. Is there a place where branch_device_type.ipynb. would fit well?

@BainanXia
Copy link
Collaborator

I vote that we save branch_device_type.ipynb to our dropbox and remove it from our repo. I think scenario_and_grid_cheatsheet.ipynb should stay, but we should get serious about doing automated checks in our CI process to make sure that when we change our codebase, we also detect when this breaks notebooks that are in the repo, so that we can either make sure to update them or decide that the updating process is not worth it and remove them.

I am not super familiar with the Dropbox structure. Is there a place where scenario_and_grid_cheatsheet.ipynb would fit well?

@danielolsen How about Dropbox (Gates Ventures)/BreakthroughEnergySciences/Results/Features? It's empty there and I'm not sure what this folder is designed for originally. It seems to fit at this point.

@danielolsen
Copy link
Contributor

I'm not sure this is results or a feature, it feels more like a reference or an onboarding tool. Maybe BreakthroughEnergySciences/Project/*?

@BainanXia
Copy link
Collaborator

I'm not sure this is results or a feature, it feels more like a reference or an onboarding tool. Maybe BreakthroughEnergySciences/Project/*?

My interpretation is the project folder is designed for separate projects other than the synthetic network. If we really want to put it there, it could be BreakthroughEnergySciences/Project/TAMU network model. The reason I choose feature folder is simply because it is a feature of the network model, i.e. how the corresponding information is stored and what does the topology look like, etc. I'm not quite care about where we put those notebooks in the dropbox as long as we keep them for potential future usage, given the current dropbox architecture is not strictly logically organized as the repo does.

Copy link
Collaborator

@BainanXia BainanXia left a comment

Choose a reason for hiding this comment

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

This is good to go. Thanks.

@rouille rouille merged commit bc7da40 into develop Nov 30, 2020
@rouille rouille deleted the ben/notebook branch November 30, 2020 20:01
@ahurli ahurli mentioned this pull request Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related to package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants