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

Refactor postprocessing #82

Open
wants to merge 34 commits into
base: dev
Choose a base branch
from
Open

Conversation

henhuy
Copy link
Collaborator

@henhuy henhuy commented Oct 21, 2022

Steps:

  • Move helper functions into own module
  • Refactor postprocessing.py to use predefined calculations
  • Switch to string-based scalars/sequences-keys
  • Make function map_var_names optional (activated by default)
  • Refactor attributes in add_component_info to be adaptable and provide default attributes

@henhuy henhuy added the enhancement New feature or request label Oct 21, 2022
@henhuy henhuy requested a review from jnnr October 21, 2022 15:09
@henhuy henhuy self-assigned this Oct 21, 2022
@henhuy
Copy link
Collaborator Author

henhuy commented Oct 21, 2022

Hey @jnnr, I started refactoring. Please look at predefined calculations and tell me if you like it that way!
We should also add docstrings to each calculation to inform users about the underlying calculation.

Copy link
Member

@jnnr jnnr left a comment

Choose a reason for hiding this comment

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

Thanks for the new approach! I really like the modularity.
For now, I have only one comment.
I can review again after the next steps.
I could check if some parts have more implicit assumptions than desirable. The goal in this PR is to reproduce the existing postprocessed results, right? So this could happen separately.


return inputs
class InvestedStorageCapacityCosts(Calculation):
Copy link
Member

Choose a reason for hiding this comment

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

Multiplying ep_costs and differentiating capacity and storage_capacity is orthogonal (the latter is just a filter). So this is a bit of duplication. If it was possible to wrap calculations, this could be more clean. But not necessary for the first refactoring.

@henhuy
Copy link
Collaborator Author

henhuy commented Nov 8, 2022

Hey @jnnr,

refactored postprocessing to use string-based indexes and get component attributes from scalar_params.
This allowed me to remove much of the reindexing functions and to simplify many functions :)
Please have a look.
Also I moved some more specific calculations from helper.py into postprocessing.py (like calculation of losses and summed_variable_costs).

@jnnr
Copy link
Member

jnnr commented Nov 24, 2022

Thanks for your work, @henhuy! I tried to reproduce the results for an oemof-B3-scenario and found these differences:

  • In the sequences, the third level of the multiindex is altered from e.g. B-ch4, B-ch4-boiler, flow to B-ch4, B-ch4-boiler, "('B-ch4', 'B-ch4-boiler_large', 'flow')"
  • Sequences "by_variable" have disappeared

Scalars did not change, which is good!

Update: This seems to happen because the first call here alters the results for the second call. Can be fixed by reverting the order, but maybe it makes sense to understand better what is happening.

data_scal, rel_paths_scal = self._get_scalars(self, es)
data_seq, rel_paths_seq = self._get_sequences(self, es)

@henhuy
Copy link
Collaborator Author

henhuy commented Nov 25, 2022

I tried to reproduce the results for an oemof-B3-scenario

Thanks for checking. Which scenario did you test? I will add scenario results to tests and try to reproduce the error.

@jnnr
Copy link
Member

jnnr commented Nov 25, 2022

I tried to reproduce the results for an oemof-B3-scenario

Thanks for checking. Which scenario did you test? I will add scenario results to tests and try to reproduce the error.

I checked with 2050-80-gas_moreCH4. You should be able to see it with the dump you already uploaded. What we do in oemof-B3 is call ResultsDataPackage.from_energysytem, which also processes the sequences.

@jnnr jnnr added this to the v0.0.2 milestone Dec 13, 2022
@henhuy henhuy force-pushed the feature/refactor_postprocessing branch from 9f05634 to f166654 Compare January 12, 2023 16:18
@henhuy henhuy marked this pull request as ready for review January 12, 2023 16:18
@henhuy henhuy marked this pull request as draft January 12, 2023 16:19
@henhuy henhuy marked this pull request as ready for review January 27, 2023 09:41
@henhuy henhuy force-pushed the feature/refactor_postprocessing branch from 75b3059 to e4aa0c0 Compare January 27, 2023 09:42
@henhuy
Copy link
Collaborator Author

henhuy commented Jan 27, 2023

Refactoring is done.
Tests are still failing - I have to check why (datapackage test, which I did not touch).

@jnnr: could you check once more if your oemof-B3-scenario returns the same results?

@jnnr
Copy link
Member

jnnr commented Feb 7, 2023

Refactoring is done. Tests are still failing - I have to check why (datapackage test, which I did not touch).

@jnnr: could you check once more if your oemof-B3-scenario returns the same results?

Have checked it and found only marginal differences (decimal round-off errors). I had to swap the order of postprocessing as suggested earlier to preserve the postprocessed timeseries.

@henhuy
Copy link
Collaborator Author

henhuy commented Feb 8, 2023

The 3.7 build fails as used frictionless version tries to import cached_property from functools (ImportError: cannot import name 'cached_property' from 'functools' (/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/functools.py)) which is not supported in python < 3.8.
Should we pin python version to >=3.8?
My opinion: As python 3.10 is out - supporting 3.8, 3.9 and 3.10 should be sufficient!
What do you think @jnnr ?

@jnnr
Copy link
Member

jnnr commented Feb 8, 2023

My opinion: As python 3.10 is out - supporting 3.8, 3.9 and 3.10 should be sufficient! What do you think @jnnr ?

I am ok with that. oemof.solph supports 3.8 -3.10, too.

@henhuy henhuy force-pushed the feature/refactor_postprocessing branch from 15b1a84 to 392ddf9 Compare February 9, 2023 10:24
@MaGering MaGering modified the milestones: v0.0.2, v0.0.3 Aug 31, 2023
@MaGering MaGering modified the milestones: v0.0.3, v0.0.4 Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants