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

Add import_ecoinvent_release utility #227

Merged
merged 8 commits into from
Nov 9, 2023
Merged

Add import_ecoinvent_release utility #227

merged 8 commits into from
Nov 9, 2023

Conversation

cmutel
Copy link
Member

@cmutel cmutel commented Nov 8, 2023

This is a big philosophical change for Brightway, as we can have multiple biosphere databases, don't need to tightly link a bw2io release with a certain ecoinvent release, and shouldn't need any updates at all for future ecoinvent releases.

The function import_ecoinvent_release assumes that the biosphere database is not yet created - in other words, you shouldn't run bw2setup(). This is because each release has a slightly different set of biosphere flows, and we want to label them separately and specifically. So instead of creating a biosphere3 database we create a e.g. ecoinvent-3.6-biosphere database.

This also enforces our naming convention for databases, so version 3.6 with APOS of ecoinvent will always be ecoinvent-3.6-apos on import.

About half the code and 99% of the pain was from internal ecoinvent inconsistencies. Anytime you have to do Levenshtein distance calculations in internal data files something has gone terribly wrong.

This currently raises an error is the biosphere flow database exists, but that will cause problems for people who want to have more than one system model from the same release installed (and only install the LCIA methods once). Not sure what to do here; we definitely want to write the appropriate data for last release requested. Maybe just overwrite the existing data with a warning? I was worried about people losing any customization of the biosphere data they already had, happy to hear ideas here.

@cmutel cmutel requested a review from tngTUDOR November 8, 2023 22:27
Copy link
Contributor

@tngTUDOR tngTUDOR 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 could add some docker based tests to make sure that all 3 major OSs [Linux, MacOS and Windows] manage to use the new feature correctly. I'm thinking about the encodings and different network transport potential issues.

bw2io/ecoinvent.py Show resolved Hide resolved
bw2io/ecoinvent.py Show resolved Hide resolved
bw2io/importers/ecospold2.py Outdated Show resolved Hide resolved
@cmutel
Copy link
Member Author

cmutel commented Nov 9, 2023

I think we could add some docker based tests to make sure that all 3 major OSs [Linux, MacOS and Windows] manage to use the new feature correctly. I'm thinking about the encodings and different network transport potential issues.

We can just use CI instead of figuring out tox plus containers 😄

@cmutel
Copy link
Member Author

cmutel commented Nov 9, 2023

This currently raises an error is the biosphere flow database exists

Fixed with use_existing_biosphere parameter.

@cmutel
Copy link
Member Author

cmutel commented Nov 9, 2023

Some test failures are due to ancient CI setups; see #228.

@cmutel cmutel merged commit 244f471 into main Nov 9, 2023
3 of 5 checks passed
@cmutel
Copy link
Member Author

cmutel commented Nov 9, 2023

@tngTUDOR Thanks a lot. Pretty proud of this, to be honest, it is a huge win for the users.

@cmutel
Copy link
Member Author

cmutel commented Nov 12, 2023

@tngTUDOR The behaviour when an existing biosphere database is present is changed in DEV26, released today. We now get:

>>> bi.ecoinvent.import_ecoinvent_release("3.8", "cutoff", 
...     biosphere_name="ecoinvent-3.7.1-biosphere", 
...     lcia=False)
Adding 95 biosphere flows to ecoinvent-3.7.1-biosphere:
        Benzovindiflupyr: ('water', 'ground-')
	Benzovindiflupyr: ('air',)
	Chlorantraniliprole: ('water', 'ground-')
	Chlorantraniliprole: ('soil', 'agricultural')
	Azoxystrobin: ('water', 'ground-')
	Carfentrazone ethyl ester: ('water', 'ground-')
	Benzovindiflupyr: ('air', 'low population density, long-term')

Much nicer than before!

@cmutel cmutel deleted the ecoinvent-import branch November 12, 2023 09:13
@IsaacZhucheng
Copy link

Hi! I saw your PR and think it might be something to do with the issue I just submitted. Seems like ecoinvent 3.10 should be supported by a different biosphere3. I am wondering if you have any idea on how can I do this? I am using bw2io(0.8.10)

@Stew-McD
Copy link
Contributor

Works well. Great work!
I have tested this function on linux with brightway 2 and 2.5 with almost all ei releases.
It didn't work for ei version < 3.4. But I guess that doesn't matter.

See here if you want to know more

2023-12-09 17:29:12,833 INFO:Importing 3.3 apos
2023-12-09 17:29:12,853 INFO:Instantiated ecoinvent_interface class:
    Class: EcoinventRelease
    Instance ID: 139759073379808
    Version: 2.4.1
    User: LUCML
    Output directory: /home/stew/.local/share/EcoinventInterface/cache
    Custom headers: False
    Custom URLs: False
    
2023-12-09 17:29:35,018 INFO:Fixing versions in unit process datasets
2023-12-09 17:30:57,332 INFO:Fixing versions in master data
2023-12-09 17:38:21,631 ERROR:FAILURE: 3.3 - apos
2023-12-09 17:38:21,632 ERROR:Exception occurred
Traceback (most recent call last):
  File "/home/stew/code/gh/bw2basics/testing/importecoinventrelease_test.py", line 56, in <module>
    bi.import_ecoinvent_release(version=version, system_model=system_model)
  File "/home/stew/code/gh/brightway2-io_bw2legacy/bw2io/ecoinvent.py", line 307, in import_ecoinvent_release
    if row[cf_col_label] is None:
KeyError: 'cf'

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.

None yet

4 participants