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

HRA stressor comparison bug on sample data #1467

Closed
dcdenu4 opened this issue Dec 11, 2023 · 3 comments · Fixed by #1478
Closed

HRA stressor comparison bug on sample data #1467

dcdenu4 opened this issue Dec 11, 2023 · 3 comments · Fixed by #1478
Assignees
Labels
critical for issues likely to obstruct the whole dev team (e.g. broken builds) in progress This issue is actively being worked on
Milestone

Comments

@dcdenu4
Copy link
Member

dcdenu4 commented Dec 11, 2023

A user on the forum noted they were getting an error when running HRA with sample data in 3.14.0.

https://community.naturalcapitalproject.org/t/error-in-hra-with-dawnloaded-sample-data/4339

It looks like when comparing stressors one table is lowercasing while the other is not. For the habitat_stressor_info table validation.get_validated_dataframe is being used which does return a table with lowercased values. For the criteria table, utils.read_csv_to_dataframe is returning a table where values are NOT lowercased.

Note: validation.get_validated_dataframe calls utils.read_csv_to_dataframe, so this is likely an unique case handling the HRA criteria table.

I can reproduce the issue with the latest invest:main

@dcdenu4 dcdenu4 added the critical for issues likely to obstruct the whole dev team (e.g. broken builds) label Dec 11, 2023
@dcdenu4 dcdenu4 added this to the 3.14.1 milestone Dec 11, 2023
@dcdenu4 dcdenu4 changed the title HRA stressor comparison bug on default data HRA stressor comparison bug on sample data Dec 11, 2023
@dcdenu4
Copy link
Member Author

dcdenu4 commented Dec 11, 2023

Interestingly, all tests pass so we must not be running a regression test on sample data. And looking at the HRA tests, all the unit tests use lowercase values for table creations.

@davemfish
Copy link
Contributor

Interestingly, all tests pass so we must not be running a regression test on sample data. And looking at the HRA tests, all the unit tests use lowercase values for table creations.

see also #1468

@dcdenu4 dcdenu4 self-assigned this Dec 11, 2023
@dcdenu4 dcdenu4 added the in progress This issue is actively being worked on label Dec 11, 2023
@dcdenu4
Copy link
Member Author

dcdenu4 commented Dec 11, 2023

So, I think that reading the table with validation.get_validated_dataframe is the issue here. That function does some additional mutation to the dataframe that helps when validating. But it causes consequences when using in the model directly. In this case, the function lowercases the values of columns which utils.read_csv_to_dataframe does not. So let's use the same utils function for reading when we're comparing.

dcdenu4 added a commit to dcdenu4/invest that referenced this issue Dec 14, 2023
dcdenu4 added a commit to dcdenu4/invest that referenced this issue Dec 14, 2023
dcdenu4 added a commit to dcdenu4/invest that referenced this issue Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical for issues likely to obstruct the whole dev team (e.g. broken builds) in progress This issue is actively being worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants