Factor out functions that check input parameters

[rouille]

Purpose

Factor out functions that check input parameters of analysis functions and gather them in a common module.

What is the code doing

I moved things around to avoid duplicates

Where to look

• a new postreise.analyze.check module has been created.
• already existing check functions from analysis modules have been moved in this new module.
• some tests have been modified

Time estimate

20min. The tests fail because PR Breakthrough-Energy/PowerSimData#273 need to be merged

[rouille]

I could probably write some tests for the check functions but since we test the input parameters of the analysis functions that use these functions I thought it was not necessary.

[jenhagg]

Are we still using _resource_func defined in curtailment.py? There are still docstrings referencing it instead of the new constants in powersimdata.

[danielolsen]

Are we still using _resource_func defined in curtailment.py? There are still docstrings referencing it instead of the new constants in powersimdata.

Yes, we use it within calculate_curtailment_time_series() so that we know that to get the profiles for both 'wind' and 'wind_offshore' we use the get_wind() method. It is another example of ugly getattr code from a few months back: https://github.com/Breakthrough-Energy/PostREISE/blob/develop/postreise/analyze/generation/curtailment.py#L100

[rouille]

Are we still using _resource_func defined in curtailment.py? There are still docstrings referencing it instead of the new constants in powersimdata.

Yes, we use it within calculate_curtailment_time_series() so that we know that to get the profiles for both 'wind' and 'wind_offshore' we use the get_wind() method. It is another example of ugly getattr code from a few months back: https://github.com/Breakthrough-Energy/PostREISE/blob/develop/postreise/analyze/generation/curtailment.py#L100

Should we have a get_wind_offshore in the Analyze class that returns the None if there is no such generators in the scenario and the profile for the wind offshore plant otherwise?

[jenhagg]

Oh I see how it's used. We might still want to update the docstring, since the default is now renewable_resources instead of _resource_func.keys(). Also, minor point, but we are introducing the requirement that those two are equal, otherwise there could be a KeyError on _resource_func - might be worth having a test to validate that.

[rouille]

Oh I see how it's used. We might still want to update the docstring, since the default is now renewable_resources instead of _resource_func.keys(). Also, minor point, but we are introducing the requirement that those two are equal, otherwise there could be a KeyError on _resource_func - might be worth having a test to validate that.

We can revert and use the keys. I used the renewable_resources list with the hope of getting rid of the dict but I gave up.

[jenhagg]

Oh I see how it's used. We might still want to update the docstring, since the default is now renewable_resources instead of _resource_func.keys(). Also, minor point, but we are introducing the requirement that those two are equal, otherwise there could be a KeyError on _resource_func - might be worth having a test to validate that.

We can revert and use the keys. I used the renewable_resources list with the hope of getting rid of the dict but I gave up.

It seems like a step in the right direction, no need to revert in my opinion (up to you though).

[danielolsen]

Should we have a get_wind_offshore in the Analyze class that returns the None if there is no such generators in the scenario and the profile for the wind offshore plant otherwise?

This might be the cleanest way to get rid of a lot of the ugliness in our codebase. Storing the 'wind_offshore' profiles within get_wind() seemed easy at the time, and stays consistent with the profiles living in e.g. usa_wind_v*.csv on the server, but there is lots of weirdness that we had to do to make everything work around this.

[rouille]

@danielolsen. Is the following:

# For simple methods:
# MWh to metric tons of CO2
# Source: IPCC Special Report on Renewable Energy Sources and Climate Change
# Mitigation (2011), Annex II: Methodology, Table A.II.4, 50th percentile
# http://www.ipcc-wg3.de/report/IPCC_SRREN_Annex_II.pdf
carbon_per_mwh = {
    "coal": 1001,
    "dfo": 840,
    "ng": 469,
}

# For curve methods:
# MMBTu of fuel per hour to metric tons of CO2 per hour
# Source: https://www.epa.gov/energy/greenhouse-gases-equivalencies-calculator-calculations-and-references
# = (Heat rate MMBTu/h) * (kg C/mmbtu) * (mass ratio CO2/C) / (kg to tonnes)
carbon_per_mmbtu = {
    "coal": 26.05,
    "dfo": 20.31,
    "ng": 14.46,
}

defined in the carbon module something that could be moved in powersimdata.network.usa_tamu.constants.plants? If yes, this can be part of PR Breakthrough-Energy/PowerSimData#273

[danielolsen]

@danielolsen. Is the following:

# For simple methods:
# MWh to metric tons of CO2
# Source: IPCC Special Report on Renewable Energy Sources and Climate Change
# Mitigation (2011), Annex II: Methodology, Table A.II.4, 50th percentile
# http://www.ipcc-wg3.de/report/IPCC_SRREN_Annex_II.pdf
carbon_per_mwh = {
    "coal": 1001,
    "dfo": 840,
    "ng": 469,
}

# For curve methods:
# MMBTu of fuel per hour to metric tons of CO2 per hour
# Source: https://www.epa.gov/energy/greenhouse-gases-equivalencies-calculator-calculations-and-references
# = (Heat rate MMBTu/h) * (kg C/mmbtu) * (mass ratio CO2/C) / (kg to tonnes)
carbon_per_mmbtu = {
    "coal": 26.05,
    "dfo": 20.31,
    "ng": 14.46,
}

defined in the carbon module something that could be moved in powersimdata.network.usa_tamu.constants.plants? If yes, this can be part of PR Breakthrough-Energy/PowerSimData#273

Yes.

[danielolsen]

The code looks good, thanks for cleaning this up. If the tests pass once the dependency issue is worked out, I think it's good to go.