Skip to content

Conversation

@finozzifa
Copy link
Contributor

@finozzifa finozzifa commented Jan 24, 2025

This pull request proposes a set of unit tests for scripts/compile_cost_assumptions.py, together with some minimal code re-factoring.

This pull request does not contain any major to the functionalities of the scripts.

Changes proposed in this Pull Request

Unit tests

The unit tests are for the functions:

  • add_description
  • annuity
  • clean_up_units
  • convert_units
  • get_data_from_DEA
  • get_excel_sheets
  • get_sheet_location
  • set_round_trip_efficiency
  • set_specify_assumptions

For introducing unit tests, I have introduce some minor changes to the functions. For example, I now pass as function arguments all snakemake.params/.config previously used in functions (but not passed as function arguments).

changes in function annuity

The function annuity is defined stand-alone and within the function add_home_battery_costs. The two versions are identical. I have removed the version within add_home_battery_costs.

Moreover, annuity implemented an if statement for discount rates that are pandas series. Considering that this case does not seem to occur, I have removed it.

numpydoc strings

I have enriched the doc-strings (following the numpydoc standard) for all functions in compile_cost_assumptions.py and compile_cost_assumptions_usa.py with the list of input arguments and outputs. I have also added static types to the function definitions.

Other changes

add logging

Added a logger and replaced print(msg) with the corresponding logger.warn/.info

shadow names from outer scopes

I have also:

  • proposed to rename the dictionary sheet_names to dea_sheet_names as I feel that the latter name is clearer
  • proposed to replace shadow names from outer scopes in the functions. For example, I have replaced
def set_specify_assumptions(tech_data):

with

def set_specify_assumptions(technology_dataframe):

AttributeError: 'NoneType' object has no attribute 'fillna' in get_data_DEA

The function get_data_DEA returns None if, for a given technology, no DEA excel file is found.

    excel_file = get_sheet_location(tech_name, sheet_names_dict, input_data_dict)
    if excel_file == "Sheet not found" or excel_file == "Multiple sheets found":
        logger.info(f"excel file not found for technology: {tech_name}")
        return None

The function get_data_DEA is then called in get_data_from_DEA. The function fill NaN with 0 as shown in the snippet below

        df = get_data_DEA(
            years,
            tech_name,
            sheet_names_dict,
            input_data_dictionary,
            offwind_no_grid_costs,
            expectation,
        ).fillna(0)

However NoneType has no attribute fillna. I therefore propose a change to fix this behavior. I have modified get_data_DEA such that it returns an empty dataframe

    excel_file = get_sheet_location(tech_name, sheet_names_dict, input_data_dict)
    if excel_file == "Sheet not found" or excel_file == "Multiple sheets found":
        logger.info(f"excel file not found for technology: {tech_name}")
        return pd.DataFrame()

Checks

I checked explicitly that the above-mentioned changes do not result in changes in the output files.

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Data source for new technologies is clearly stated.
  • Newly introduced dependencies are added to environment.yaml (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the GPLv3 license.

@lkstrp
Copy link
Member

lkstrp commented Feb 5, 2025

@finozzifa is doing some great and needed refactoring here. I guess we do not need backwards compatible argument names/ be as strict as we are in PyPSA/ linopy @FabianHofmann ?

@FabianHofmann
Copy link

FabianHofmann commented Feb 5, 2025

no, not at all. this is a rather a data processing tool to produce final reusable results rather than a package used in scripts. meaning, let's go :)

@finozzifa finozzifa marked this pull request as ready for review February 17, 2025 14:12
@finozzifa finozzifa changed the title Unit test compile cost assumptions Unit test compile cost assumptions - Part 1 Feb 17, 2025
Copy link
Member

@lkstrp lkstrp left a comment

Choose a reason for hiding this comment

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

Thanks again for tons of docstrings, types and tests! I skimmed most parts, just two comments:

Could you maybe move the mocked data fixtures away from conftest? Maybe into something like mocked_data.py and import it from there. Otherwise it will bloat conftest.

And a general note on naming: Especially when typed, I prefer something like years: list over list_of_years: list. It is more concise and readable, and it is overly descriptive for typed variables.

@lkstrp lkstrp merged commit ee37d71 into PyPSA:master Feb 19, 2025
3 checks passed
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