Skip to content

Commit

Permalink
for sequential attributions, use index to infer skipping of steps rat…
Browse files Browse the repository at this point in the history
…her than re-assigning `data_format` which leads to mismatch between FB tybe and data_format and causes errors in subsequent steps
  • Loading branch information
bl-young committed Sep 8, 2023
1 parent efa8513 commit e7b51a2
Showing 1 changed file with 4 additions and 6 deletions.
10 changes: 4 additions & 6 deletions flowsa/flowby.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ def attribute_flows_to_sectors(
if isinstance(attribute_config, dict):
attribute_config = [attribute_config]

for step_config in attribute_config:
for index, step_config in enumerate(attribute_config):
validate = True
grouped: 'FB' = (
self
Expand All @@ -657,7 +657,9 @@ def attribute_flows_to_sectors(
if len(grouped) == 0:
log.warning(f'No data remaining in {self.full_name}.')
return self
if self.config['data_format'] in ['FBA', 'FBS_outside_flowsa']:
if index > 0:
fb = grouped.copy()
elif self.config['data_format'] in ['FBA', 'FBS_outside_flowsa']:
fb: 'FlowByActivity' = (
grouped
.map_to_sectors(external_config_path=external_config_path)
Expand Down Expand Up @@ -793,10 +795,6 @@ def attribute_flows_to_sectors(
'descendants'], errors='ignore')
.drop(columns=step_config.get('drop_columns', []))
)
# reset datatype to FBS because otherwise when we loop through
# to the next attribution method, the FBA will be re-cleaned and
# have additional sector columns appended
self.config.update({'data_format': 'FBS'})

return self

Expand Down

2 comments on commit e7b51a2

@bl-young
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@catherinebirney FYI, with my update in f64887b, the water FBS which uses sequential attribution was having issues with fb = grouped.sector_aggregation(). data_format was FBS, but it was not an FBS so it couldn't do sector_aggregation(). I think this commit is a bit more robust and does what you want but let me know if you disagree.

@catherinebirney
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah great, that looks good - its a better approach

Please sign in to comment.