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

Create ScenarioInfo class #94

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Create ScenarioInfo class #94

merged 4 commits into from
Feb 28, 2020

Conversation

BainanXia
Copy link
Collaborator

@BainanXia BainanXia commented Feb 21, 2020

This PR consists of three parts:

  1. An empty __init__.py is added to the tests folder to fix the issue when using mock_scenario object in PostREISE, it complains not finding mock_grid in PowerSimData

  2. All functions and attributes of scenario_info object is in a single python script, scenario_info.py under scenario folder. Discussed with @rouille , both of us think the user should be able to access scenario_info in a general sense not just use it to calculate target capacities in scenario design.

  3. A full test of all the functions of the scenario_info object is implemented in test_scenario_info.py. All 6 functions passed the tests via mock scenario.

Note that I noticed the commit message should be a 'feat' instead of 'fix', a force push is conducted to fix this.

@rouille
Copy link
Collaborator

rouille commented Feb 21, 2020

We should create a tests folder in PowerSimData/powersimdata/scemario and move the test_scenario_info.py there. The tests folder in PowerSimData/powersimdata/ houses the mock objects that are used in the tests.

@rouille
Copy link
Collaborator

rouille commented Feb 21, 2020

Some lines are too long. All lines should be < 80 characters (PEP8).

'wind': wind,
'hydro': hydro
}

Copy link
Collaborator

@rouille rouille Feb 21, 2020

Choose a reason for hiding this comment

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

I would recommend to write a _check_state function that makes sure that the scenario instance that you pass to the constructor is in the analyze state (scenario.state.name == analyze). Otherwise raise an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I did not add this after our discussion is it fails the tests. It seems the MockAnalyze object has no 'name' attribute.

AttributeError: 'MockAnalyze' object has no attribute 'name'

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I had been faking the class so you can import Analyze and do an isinstance on the mock, but I didn't enable the lower-tech way of just checking the name attribute. We should probably add this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

self.name = 'analyze' in MockAnalyze should do it

print("%s is incorrect." % area)
raise Exception('Invalid area')
return loadzone_set

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can be more specific here and raise a ValueError inplace of a standard Exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, area supports loadzone, state, state abbreviation, interconnect and 'all'. Potentially, it will be more in the future, such as combinations of interconnect, state or loadzones. Not sure whether we should put the specific guidance here or in function docs.

print('start_time falls behind end_time!')
raise Exception('Invalid time range')
return start_i, end_i

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. A ValueError would be more explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I simply change Exception to ValueError here? I was thinking about several conditions:

  1. start_time and end_time are in wrong format or some random strings.
  2. start_time and end_time are in right format but not in the valid range of the scenario
  3. start_time and end_time are in right format but in wrong order, i.e. should be switched

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say that ValueError points to wrong argument(s). This would conceren #2 and #3. If you want to check the type you can add another condition that uses isinstance and raise a TypeError if not str.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just saw the changes you made. Looks good.

'plant_id': [101, 102, 103, 104, 105, 106],
'bus_id': [1001, 1002, 1003, 1004, 1005, 1006],
'type': ['solar', 'wind', 'ng', 'coal', 'dfo', 'hydro'],
'zone_name': ['zone1', 'zone1', 'zone1', 'zone1', 'zone1', 'zone1'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure this test data covers more edge cases: e.g. 2+ zones, some resources are in both zones, some are in just one or the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's exactly what I did originally and I figured out that in MockGrid we only have one zone in the zone2id attribute. This test uses MockScenario which calls MockAnalyze then takes MockGrid. We could add attributes to the grid but seems not be able to overwrite the existing attributes in the grid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's expand MockGrid to have at least two zones/ids natively.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add other values in the zoneid dict of the MockGrid object?

mock_wind = mock_pg[wind_plant_id] * 4
mock_hydro = mock_pg[hydro_plant_id] * 1.5

start_time = '2016-01-01 00:00:00'
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, let's have our dates span more than one day, so that we can ensure that check_time_range is comparing full datetimes and not just times. E.g. 6am one day to 3am the next day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. The function is comparing the index order of the start time and the end time, thus it should work. But you are right we could span the mock profiles to longer range. The down side is it makes the tests less eyeball tractable.

total_capacity = self.grid.plant[(self.grid.plant['type'] == gentype) &
(self.grid.plant['zone_name'].isin(loadzone_set))]['Pmax'].sum()
if total_capacity == 0:
print('Warning: no such type of generator in the area specified!')
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is the warnings package (https://docs.python.org/3/library/warnings.html#module-warnings) that could be useful here.

elif area in {'Texas', 'Western', 'Eastern'}:
loadzone_set = interconnect2loadzone[area]
elif area == 'all':
loadzone_set = list(self.grid.zone2id.keys())
Copy link
Contributor

Choose a reason for hiding this comment

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

This option does not return a set: we should wrap the grid keys in set() instead of 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.

Good catch.

"""
total_generation = self.get_generation(gentype, area, start_time, end_time)
total_profile_resource = self.get_profile_resource(gentype, area, start_time, end_time)
if total_profile_resource == 0 and total_generation == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Good thinking ahead.

total_hours = end_i - start_i + 1
total_capacity = self.get_capacity(gentype, area)
if total_capacity == 0:
raise ZeroDivisionError('Division by zero')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this create an exception, or return 0? Either one seems okay, but it seems strange to me that it doesn't match get_curtailment

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 thought about this before. The idea is to separate between a reasonable zero and an irregular zero:
--A reasonable zero: 1) zero profile, zero generation, thus zero curtailment 2) zero generation, nonzero capacity, zero capacity factor
--An irregular zero: zero generation, zero capacity, also zero capacity factor.
Or we have a better way to handle this difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you've thought about it in the context of how we will use this object/method, I'm happy. If we can run the capacity scaling successfully in Western, including zones like WA where there's no solar, then I don't have strong feelings on the underlying implementation. Maybe a more descriptive message saying that the zero comes from zero capacity for the cases where we do run into this error.

@BainanXia
Copy link
Collaborator Author

Some lines are too long. All lines should be < 80 characters (PEP8).

Done with this.

@BainanXia
Copy link
Collaborator Author

Should I modify MockGrid and MockAnalyze (in PostREISE) as suggested and create two separate PRs at this point @rouille @danielolsen ?

@danielolsen
Copy link
Contributor

@BainanXia yes, go ahead.

@BainanXia
Copy link
Collaborator Author

The current version of this PR:
-Done with all the suggested changes and pep8 cleanings so far
-Modified MockGrid(in PowerSimData) and MockAnalyze (in PostREISE) to cooperate with scenario_info tests
-Rebased this PR after changes in MockGrid were merged and force-pushed after the rebase
-Add get_demand function and corresponding tests. Now scenario_info object is capable to calculate total demand of a specific area during valid time range of a given scenario.

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.

Great work
Check the following link: https://github.com/intvenlab/REISE/wiki/Pull-Request-Etiquette. It has been written by Merielle and it standardized the PR process.

@BainanXia BainanXia merged commit 74768f8 into develop Feb 28, 2020
@BainanXia BainanXia deleted the add_scenario_info branch February 28, 2020 01:35
@ahurli ahurli mentioned this pull request Mar 11, 2021
@rouille rouille changed the title fix: add scenario_info object and corresponding tests Create Scenarioinfo class Mar 16, 2021
@rouille rouille changed the title Create Scenarioinfo class Create ScenarioInfo class 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