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

Feature/routine for decomissioning of exisiting capacity #78

Merged
merged 30 commits into from
Jun 25, 2024

Conversation

FelixMau
Copy link
Contributor

@FelixMau FelixMau commented Mar 15, 2024

Adding in Decommissioning for stock processes fading out.

  • Detect processes (via "0" name end)
  • Check on existing max values
  • Unify capacity values to one largest capacity occouring

@FelixMau FelixMau self-assigned this Mar 15, 2024
@nailend nailend changed the title Feature/routine for decomissioning of exisitng capacity Feature/routine for decomissioning of exisiting capacity Mar 27, 2024
@FelixMau FelixMau requested review from henhuy and SabineHaas April 10, 2024 13:53
Base automatically changed from feature/add_mimo_adapter to dev April 15, 2024 10:22
Comment on lines 68 to 81
if max_column in adapter_dict.keys():
if adapter_dict[capacity_column] == adapter_dict[max_column]:
adapter_dict[max_column] = adapter_dict[capacity_column] / np.max(
adapter_dict[capacity_column]
)
else:
logging.info("Decommissioning and max value can not be set in parallel")
adapter_dict[max_column] = list(
(adapter_dict[max_column] / np.max(adapter_dict[capacity_column]))
)
else:
adapter_dict[max_column] = adapter_dict[capacity_column] / np.max(
adapter_dict[capacity_column]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain whats happening in decommission function, as I don't quite get it from code alone?
Best to put in docstring directly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Baisicly I am just dividing the yealy changing capacity by the maximum capacity and then setting the max capacity as value for all years while using the quotient as value for max

I also wanted to check whether max value already is used and wheter it just is the same as the capacity value (as in data provided for steel industy)

@FelixMau FelixMau marked this pull request as draft April 15, 2024 14:16
@FelixMau
Copy link
Contributor Author

Separating caluclaitons to happen pre and post calculation.

Recalculation of activity bonds to be relative values. And then applying decommission logic helps to be certain about already existing max values.

@FelixMau FelixMau linked an issue Apr 15, 2024 that may be closed by this pull request
Moreover making it easier to adapt in future if more unexpected values occour.
…g-capacity

# Conflicts:
#	data_adapter_oemof/adapters.py
Copy link
Contributor

@henhuy henhuy left a comment

Choose a reason for hiding this comment

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

Hey Felix, thank you for starting to implement this.
I made some comments/requests for you.
One thing: the max/min in MIMO will look different and will not depend on output_parameters - just to keep that in mind.

data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/adapters.py Outdated Show resolved Hide resolved
data_adapter_oemof/adapters.py Outdated Show resolved Hide resolved
@FelixMau
Copy link
Contributor Author

Made following changes:

  • Rearranged Functions for better overview & understanding
  • Implemented pre mapping calculations (to normalize activity bonds and map fix to min and max)
  • Revisited Decomissioning function and adapting to now known input if max value was already existing

@FelixMau FelixMau marked this pull request as ready for review April 17, 2024 11:12
@FelixMau
Copy link
Contributor Author

Will be adapting tests on dev to keep things at least a little separated.
Going to add empty dictionaries for input and output parameters

@FelixMau FelixMau requested a review from henhuy May 6, 2024 09:07
Copy link
Contributor

@henhuy henhuy left a comment

Choose a reason for hiding this comment

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

Implementation looks good.
I have only smaller things regarding user feedback via logging, which could help us debugging.

data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
data_adapter_oemof/calculations.py Outdated Show resolved Hide resolved
@FelixMau FelixMau requested a review from henhuy May 23, 2024 07:43
@FelixMau
Copy link
Contributor Author

Now including a nan value handler to replace nan values automatically if the parameter values differ over the periods.

Replacing max values by 9999999999999.
Replacing min and all other values by 0.

This feature is not meant to be used extensively since leaving values empty for some years is implicit and in most cases considered 0!

Copy link
Contributor

@henhuy henhuy left a comment

Choose a reason for hiding this comment

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

Have two questions

if column in ["method", "source", "comment", "bandwidth_type"]:
continue

if group_df[column].nunique(dropna=False) > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should make sure to only apply cahnges to columns with nan values. I modified to make the intention more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow, GitHub marked the wrong line. I meant the part if group_df[column].nunique(dropna=False) > 1: ?

if group_df[column].nunique(dropna=False) > 1:
if column in max:
group_df[column].fillna(max_value, inplace=True)
elif column in min:
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be removed, as next else statement looks the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified to raise an error if nan values are found in columns that cannot be derived as min/max.

Multi-Period cannot handle nan values for some Periods only as far as I know.

@@ -214,10 +214,16 @@ def handle_nans(group_df: pd.DataFrame) -> pd.DataFrame:
if column in ["method", "source", "comment", "bandwidth_type"]:
continue

if group_df[column].nunique(dropna=False) > 1:
if group_df.isna().sum>0:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this check completely as I think this costs more than simply fillna all columns which need to be checked...

Comment on lines 223 to 229
if "type" in group_df.columns:

raise ValueError(f"In column {column} for process {group_df['type']} nan values are found"
f"please make sure to only provide complete datasets")
else:
raise ValueError(f"In column {column} nan values are found"
f"please make sure to only provide complete datasets")
Copy link
Contributor

Choose a reason for hiding this comment

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

This could cause us more trouble then letting None in there IMO.
So maybe not check for it instead...

Copy link
Contributor Author

@FelixMau FelixMau Jun 19, 2024

Choose a reason for hiding this comment

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

Yes it would but yet I am facing this issue again now with x2x dataset and it is hard to guess which dataset is faulty. The dataset has empty lists where also non empty lists are.
This data set has nans and lists in the same column.

Having an Error here or in data_adapter would help since it is actually an error? We should not ignore and detect it somewhere (as early as possible)?

But maybe this should be discussed in another issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having an Error here or in data_adapter would help since it is actually an error? We should not ignore and detect it somewhere (as early as possible)?

But I think there will be many cases in datasets where this is the case - and I don't see people fixing datasets quick enough. So better be prepared for None values and ignore them as far as possible (tabular sets default values in most cases)?

Copy link
Contributor Author

@FelixMau FelixMau Jun 20, 2024

Choose a reason for hiding this comment

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

Tabular will not be able to set default values since currently it is impossible to know which values are missing. We are loosing information if single entries are missing because tabular is receiving a list for changing values. If the list is shorter than the amount of periods we will not know anymore which years value has gone missing.

Maybe we can adjust the reading of datapackage in tabular to use pass a nan if a string is (eg. "nan") is found in such a list?

@FelixMau FelixMau force-pushed the feature/routine-for-decomissioning-of-exisitng-capacity branch from 92635d0 to d9e872d Compare June 24, 2024 12:05
@FelixMau FelixMau mentioned this pull request Jun 24, 2024
@FelixMau
Copy link
Contributor Author

FelixMau commented Jun 24, 2024

Due to unrelated changes returning to #78 d9e872d

Continueing to work on handling nans on #89

@FelixMau FelixMau merged commit ffa4845 into dev Jun 25, 2024
4 checks passed
@FelixMau FelixMau deleted the feature/routine-for-decomissioning-of-exisitng-capacity branch June 25, 2024 07:13
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.

Routine for Decomissioning of exisitng capacity
3 participants