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

Compatibility with regionalization #186

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

cmutel
Copy link
Contributor

@cmutel cmutel commented Sep 12, 2018

bw2regional namespaces locations using tuples, just like activity keys. So e.g. "Europe with Switzerland" becomes ("ecoinvent", "Europe with Switzerland"). The current code doesn't like these tuples, so this PR casts them to strings. This effectively makes them read only (storing them as strings would break the functionality in regionalized calculations), but that is fine for now.

Before merging, this PR should be reviewed carefully by someone who understands the AB code base better than me; I tried to be careful but it is entirely possible that I am missing something important, or violating some hidden assumptions.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 53.531% when pulling d8a03ee on cmutel:location-strings into 2852af5 on LCA-ActivityBrowser:master.

@bsteubing
Copy link
Member

Hi Chris, I haven't tested it, but I think your PR should work as intended. However, I am a bit hesitant to directly accept it for the following reasons:

  • it is an edge case: in most cases data will not come from bw2regional and then this is not needed
  • it makes the code slightly more complicated and requires that we remember to always treat the "location" parameter as a special case
  • there is probably a good reason, why you put a database and a location in a tuple, e.g. ("ecoinvent", "Europe with Switzerland"), but strictly speaking this is not a location anymore, but a combination of two properties... Wouldn't it be the easiest, if activity.get("location") would also return a location in bw2regional?

@cmutel
Copy link
Contributor Author

cmutel commented Sep 13, 2018

Hi Bernhard-

I understand your reluctance, and even agree to some degree. Complexity should be avoided whenever possible. However, I would like to point out that the with the current code, the AB is completely unusable for databases where regionalized LCA has been done once, i.e. you can't browse datasets or do site-generic LCA.

The reason you need to put location identifiers into a namespace is that LCIA maps don't use unique identifiers. For example, a map of watersheds would have id values 1, 2, and 3, and a map of ecoregions would also have id values 1, 2, and 3. It is just like the (database, code) identifier for activities.

Merging this PR would not change any of the work that you or other maintainers are doing, and would not really mean that you would need to think about it in the future - casting a string to a string is a no-op. I can always discover problems in the future and find fixes for them when they occur.

@haasad
Copy link
Member

haasad commented Sep 14, 2018

Hey guys,

I just tested this PR and everything works as intended, as far as I can tell. I see potential conflicts when we are going to merge the work of @tmillross, because this affects editing of activities and read-only databases.

@cmutel, is there some kind of identifier that a database is used in bw2regional? I would propose to force bw2regional dbs to be read-only as soon as we have merged #178. Like this we allow users to view/calculate bw2regional databases without running into problems related to editing the location of an activity.

In the meantime I'm favour of merging this PR anyway, even if it makes the code a bit more complicated. In my opinion the benefit of being able to do new things with the AB (ie using regionalized LCA) is completely worth the effort. And if we don't support things like that they might stay an "edge case".

@bsteubing
Copy link
Member

Hi both, I am fine with merging this PR. I still don't fully understand why a location would be something like ("ecoinvent", "Europe with Switzerland"), because only the second element is a geographical reference (and hence my question if this shouldn't be altered in bw2regional). But I trust you that this somehow makes sense, so let's go for it.

@bsteubing
Copy link
Member

I will try to reserve a week to integrate all the changes that we have in the pipeline (e.g. Tom's work) in October.

@bsteubing bsteubing merged commit 3980a85 into LCA-ActivityBrowser:master Sep 14, 2018
@cmutel
Copy link
Contributor Author

cmutel commented Sep 17, 2018

Added an issue to bw2-regional to mark databases as regionalized.

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