Skip to content

Add missing wild_foods_valid_instances in the consolidated_instances … #145

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 3, 2025
Merged

Add missing wild_foods_valid_instances in the consolidated_instances … #145

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b328a1b
Add missing wild_foods_valid_instances in the consolidated_instances …
girumb Aug 25, 2025
b3f68f6
Blackify fixture.py see HEA-677
girumb Aug 26, 2025
d340f51
Merge branch 'main' into HEA-677/Key_error_'Fishing'_in_consolidated_…
girumb Aug 29, 2025
53a4d5b
Add model keys 'Fishing' and 'WildFoodGathering' see HEA-677
girumb Sep 1, 2025
b789019
Merge branch 'main' into HEA-677/Key_error_'Fishing'_in_consolidated_…
girumb Sep 3, 2025
b6474bb
Address PR feedback see HEA677
girumb Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pipelines/assets/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,9 @@ def consolidated_fixture(
for model_name, instances in livelihood_activity_valid_instances.items()
if model_name != "WealthGroup"
},
**wild_foods_valid_instances,
Copy link
Contributor

@rhunwicks rhunwicks Aug 29, 2025

Choose a reason for hiding this comment

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

@girum-air why is this required? Doesn't the code below on line 345 add the wild_foods_valid_instances?

Copy link
Contributor Author

@girum-air girum-air Sep 1, 2025

Choose a reason for hiding this comment

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

@rhunwicks the keys Fishing and WildFoodGathering weren't in the consolidated_instances at the line and that raises key error - Key error 'Fishing' in consolidated_fixture asset when trying to append it using += We might either need the keys added with empty list to initialize, I think that is better and updated it

}

Copy link
Contributor

@rhunwicks rhunwicks Sep 2, 2025

Choose a reason for hiding this comment

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

Would this be better as:

    # Add the wild foods and other cash income instances, if they are present
    for model_name, instances in {**other_cash_income_valid_instances, **wild_foods_valid_instances}.items():
        if instances and model_name != "WealthGroup":
            if model_name not in consolidated_instances:
                consolidated_instances[model_name] = []
            consolidated_instances[model_name] += instances

That way, if we ever have to add additional models in the future it will still work.

Copy link
Contributor Author

@girum-air girum-air Sep 3, 2025

Choose a reason for hiding this comment

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

Ok, agreed. Pushed the updates

# Add the wild foods and other cash income instances, if they are present
for model_name, instances in {**other_cash_income_valid_instances, **wild_foods_valid_instances}.items():
if instances and model_name != "WealthGroup":
Expand Down