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

Refactor geographical part of model immutables #692

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Refactor geographical part of model immutables #692

merged 3 commits into from
Oct 24, 2022

Conversation

rouille
Copy link
Collaborator

@rouille rouille commented Oct 12, 2022

Pull Request doc

Purpose

The main purpose is to factor out the building of the geographical part of the model immutables from the TUB class so it can be dhared by any PyPSA Netork object. The refactor adds flexibity by allowing to instantiate a ModelImmutables class from a CSV file stored on disk (as currentlt done for the usa_tamu grid model) or a PyPSA Network object for any grid model

Also, generator/storage related constants were moved in a new powersimdata/network/constants/technology folder and refactor geographical part of the model immutables following .same logic than generators.

What the code is doing

The starting point is a data frame enclosing minimal information on mappings that is augmented using some constants for a region (USA and EU) given in the powersimdata.network.constants.region.geography module. All the mappings are then derived from this data frame.

Testing

Some existing tests were refactored to account for the new logic

Where to look

A lot of new module with very little code. Small classes are used to break up USA vs EU for different mappings (interconnect, state/country, loadzone)

The trickiest part was to replace ERCOT with Texas and reorder the names (ERCOT_Eastern --> Eastern Texas) for the usa_tamu model since all constants uses ERCOT. This is done in the USA class of the `powersimdata.network.constants.region.geography module.

Usage Example/Visuals

N/A

Time estimate

30min

@rouille rouille added the refactor Code that is being refactored label Oct 12, 2022
@rouille rouille requested review from BainanXia and jenhagg October 12, 2022 06:29
@rouille rouille self-assigned this Oct 12, 2022
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.

It's much cleaner 🚀 One minor comment is not sure we would like to call the subfolder "technology", maybe "component" makes more sense? Another comment out of the scope of this PR is we might get rid of the concept of load zones in the future to support multilayer spatial resolution co-exist and let the user specify distributing demand based on which one with corresponding demand profile selected during scenario creation.

@rouille
Copy link
Collaborator Author

rouille commented Oct 13, 2022

It's much cleaner 🚀 One minor comment is not sure we would like to call the subfolder "technology", maybe "component" makes more sense? Another comment out of the scope of this PR is we might get rid of the concept of load zones in the future to support multilayer spatial resolution co-exist and let the user specify distributing demand based on which one with corresponding demand profile selected during scenario creation.

I can rename the folder to component. We could also used carrier as PyPSA. Regarding your second comment, we will likely have to refactor a lot of section of the code in PowerSimData/PostREISE to enable this new feature. We will think about it when it comes our way.

@rouille
Copy link
Collaborator Author

rouille commented Oct 13, 2022

It's much cleaner 🚀 One minor comment is not sure we would like to call the subfolder "technology", maybe "component" makes more sense? Another comment out of the scope of this PR is we might get rid of the concept of load zones in the future to support multilayer spatial resolution co-exist and let the user specify distributing demand based on which one with corresponding demand profile selected during scenario creation.

I can rename the folder to component. We could also used carrier as PyPSA. Regarding your second comment, we will likely have to refactor a lot of section of the code in PowerSimData/PostREISE to enable this new feature. We will think about it when it comes our way.

What do you think @BainanXia, component or carrier?

@BainanXia
Copy link
Collaborator

It's much cleaner 🚀 One minor comment is not sure we would like to call the subfolder "technology", maybe "component" makes more sense? Another comment out of the scope of this PR is we might get rid of the concept of load zones in the future to support multilayer spatial resolution co-exist and let the user specify distributing demand based on which one with corresponding demand profile selected during scenario creation.

I can rename the folder to component. We could also used carrier as PyPSA. Regarding your second comment, we will likely have to refactor a lot of section of the code in PowerSimData/PostREISE to enable this new feature. We will think about it when it comes our way.

What do you think @BainanXia, component or carrier?

I don't have strong preferences between component and carrier. They both sounds meaningful and I'm ok with either.

@rouille rouille force-pushed the ben/mi branch 3 times, most recently from 9d6dea1 to 4518588 Compare October 19, 2022 05:08
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.

Thanks

@rouille rouille merged commit 7543031 into develop Oct 24, 2022
@rouille rouille deleted the ben/mi branch October 24, 2022 19:32
@jenhagg jenhagg mentioned this pull request Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code that is being refactored
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants