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.

Sign up for GitHub

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

Jump to bottom

feat: support getting profiles from execute state #523

Merged
merged 4 commits into from
Jul 20, 2021
Merged

feat: support getting profiles from execute state #523

merged 4 commits into from
Jul 20, 2021

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented Jul 13, 2021 •
edited
Loading

Purpose

Address #522

What the code is doing

Define a new State subclass with methods to access transformed profiles, and have Execute and Analyze inherit from that.

Testing

Checked some of the methods for:

  • an existing scenario (analyze state)
  • a newly created scenario (create/execute state)

Time estimate

10 min

@jenhagg jenhagg requested review from danielolsen, kasparm, rouille, ahurli and BainanXia and removed request for kasparm July 13, 2021 18:23
@jenhagg jenhagg marked this pull request as draft July 13, 2021 18:31
@jenhagg
Copy link
Collaborator Author

jenhagg commented Jul 13, 2021

Working on reconfiguring the exported methods

@rouille
Copy link
Collaborator

rouille commented Jul 13, 2021

We should extend this to the Create class where we also do get_solar, etc.

rouille
rouille reviewed Jul 14, 2021
View reviewed changes
powersimdata/scenario/ready.py Outdated

def _update_scenario_info(self):
"""Updates scenario information."""
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it work? Where is the update happening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When called from analyze state, we default to this. But execute overrides the method, so it will still download the scenario list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, I'm working on moving this to state.py instead, so it can also be used from the create state, which also defines its own version of _update_scenario_info

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Scenario class has a print_scenario_info method. Just wondering if the inheritance is not going to break somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was concerned about that. After testing again it seems to work fine. Changed it so print_scenario_info is exported from scenario.state, so the Scenario doesn't have to explicitly proxy.

@jenhagg jenhagg force-pushed the jon/get_profiles branch 2 times, most recently from d7c43fb to 8ae4fa8 Compare July 14, 2021 23:43
@jenhagg
Copy link
Collaborator Author

jenhagg commented Jul 14, 2021

We should extend this to the Create class where we also do get_solar, etc.

Thinking we save that for a future PR. I don't want to do anything too crazy with inheritance without a bit more thought regarding the design.

@jenhagg jenhagg marked this pull request as ready for review July 14, 2021 23:55
@BainanXia
Copy link
Collaborator

We should extend this to the Create class where we also do get_solar, etc.

Thinking we save that for a future PR. I don't want to do anything too crazy with inheritance without a bit more thought regarding the design.

Agree. Implementing capabilities of getting time series profiles in create state is a little tricky given the fact that it is possible the user hasn't set corresponding profiles yet or transformed profiles need to be generated based on an incomplete change table, which fits into a separate PR.

@jenhagg jenhagg force-pushed the jon/get_profiles branch from 8ae4fa8 to 363ce3b Compare July 15, 2021 21:01
@jenhagg jenhagg force-pushed the jon/get_profiles branch from 363ce3b to 1949283 Compare July 16, 2021 21:52
danielolsen
danielolsen reviewed Jul 20, 2021
View reviewed changes
powersimdata/scenario/state.py
@@ -28,6 +29,19 @@ def refresh(self, scenario):
"""
pass

def _update_scenario_info(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I see what's going on here, but maybe it would be clearer if we add a comment that this method is overwritten in the Execute class, and that's why we have a placeholder here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the docstring to be more useful, but didn't mention specific classes so if things change later the description won't also need to change.

danielolsen
danielolsen approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@danielolsen danielolsen left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

BainanXia
BainanXia approved these changes Jul 20, 2021
View reviewed changes
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.

LGTM. A presume a subsequent PR would be dealing with getting time series at create state.

@jenhagg jenhagg force-pushed the jon/get_profiles branch from 1949283 to aebf5c6 Compare July 20, 2021 20:56
@jenhagg jenhagg merged commit b39d5d7 into develop Jul 20, 2021
@jenhagg jenhagg deleted the jon/get_profiles branch July 20, 2021 21:00
@jenhagg jenhagg linked an issue Aug 16, 2021 that may be closed by this pull request
Enable getting profiles from scenario in Execute state #522
Closed
1 task
@rouille rouille mentioned this pull request Aug 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouille rouille rouille left review comments

@danielolsen danielolsen danielolsen approved these changes

@BainanXia BainanXia BainanXia approved these changes

@ahurli ahurli Awaiting requested review from ahurli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable getting profiles from scenario in Execute state
4 participants
@jenhagg @rouille @BainanXia @danielolsen