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

fix: Compare impacts by subimpacts and run the comparison during export #38

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

ccomb
Copy link
Collaborator

@ccomb ccomb commented Feb 5, 2025

🔧 Problem

The current comparison graphs don't display useful informations, the impacts seem the same across all sub-impacts
image

The comparison is a separate command and is not always launched during export. It is not included in the npm commands and may end up being abandonned.

🍰 Solution

  • Refactor the export to also run the comparison graphs
  • Fix the graphs to display sub-impacts
  • use only normalization in comparison graphs

🏝️ How to test

Run the export (make export / npm run export:all) and check there are comparison graphs.
Check that the export still run if simapro is not available.

@ccomb ccomb changed the title Compare impacts by subimpacts and run the comparison during export fix: Compare impacts by subimpacts and run the comparison during export Feb 5, 2025
@ccomb ccomb force-pushed the refactor_comparison branch from da587a2 to b1c97ca Compare February 5, 2025 22:37
@ccomb ccomb requested a review from vjousse February 6, 2025 10:08
@ccomb ccomb requested a review from paulboosz February 6, 2025 10:39
@paulboosz
Copy link
Collaborator

How can I check with simapro unavailable ?

@ccomb
Copy link
Collaborator Author

ccomb commented Feb 13, 2025

How can I check with simapro unavailable ?

You connect to the simapro machine and stop the API then relaunch an export.
You should get n output like:

2025-02-13T15:37:49.764259+0100 INFO 51/86: getting impacts from Brightway for: electricity, medium voltage//[ET] market for electricity, medium voltage
2025-02-13T15:37:50.183820+0100 INFO 52/86: getting impacts from SimaPro for: electricity, medium voltage//[KH] market for electricity, medium voltage
2025-02-13T15:37:52.201775+0100 WARNING SimaPro did not answer! Is it started?
2025-02-13T15:37:52.202202+0100 WARNING SimaPro FAILED: {}

Copy link
Collaborator

@paulboosz paulboosz left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Collaborator

@vjousse vjousse left a comment

Choose a reason for hiding this comment

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

I think we should use functions when we can to avoid code duplication. And what about adding an option to generate the graphs or to disable their generation?

common/__init__.py Outdated Show resolved Hide resolved
common/__init__.py Show resolved Hide resolved
common/export.py Outdated Show resolved Hide resolved
@ccomb
Copy link
Collaborator Author

ccomb commented Feb 13, 2025

And what about adding an option to generate the graphs or to disable their generation?

Good idea, I've added an option to an internal function, and exported this option to the output process. So now the plot are generated when running for instance npm run export:textile -- --plot

@ccomb ccomb requested a review from vjousse February 13, 2025 22:00
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