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

ELEX-2763: Estimandizer update #73

Closed
wants to merge 5 commits into from
Closed

Conversation

dmnapolitano
Copy link
Contributor

Description

The changes in this PR refine the changes made in PR #59. Hopefully the code and logic are much cleaner now. Thanks to @lennybronner for making most of these changes lol and the helpful feedback 😄 🙌🏻

Note that, as a result of these changes, a change will need to be made to the test bed; specifically, in modeltestbed/redo.py, include_results_estimand=True will need to be added to the constructor. Previously, I was trying to avoid having to modify the test bed, but that lead to some ugliness 🙈. But this change is extremely minimal and the rest of the code is much cleaner now, so IMO, it's worth it.

Jira Ticket

ELEX-2763

Test Steps

tox and you can also modify the test bed per the above (until the test bed itself is modified, of course). Thanks!

@dmnapolitano dmnapolitano requested a review from a team as a code owner September 15, 2023 14:59
@dmnapolitano
Copy link
Contributor Author

Hi @lennybronner , I'm stuck and need help with the integration tests ☹️

The one failing is this one:

elexmodel 2021-11-02_VA_G --estimands=dem --office_id=G --geographic_unit_type=precinct --percent_reporting 60 --historical --aggregates=county_classification

The issue is that baseline_dem is present but not results_dem, so the Estimandizer is trying (reasonably) to estimandize a results_dem and failing due to a lack of dem estimandizer. What do we want to do in this situation? Do we want to copy baseline_dem to results_dem? This is being called from MockLiveDataHandler 🤔

Copy link
Collaborator

@lennybronner lennybronner left a comment

Choose a reason for hiding this comment

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

This looks v good! Though atm the integration tests are failing. Also when I run a historical election all subunits are unexpected.

src/elexmodel/client.py Show resolved Hide resolved
src/elexmodel/handlers/data/Estimandizer.py Show resolved Hide resolved
src/elexmodel/handlers/data/Estimandizer.py Outdated Show resolved Hide resolved
src/elexmodel/handlers/data/Estimandizer.py Show resolved Hide resolved
tests/handlers/test_estimandizer.py Show resolved Hide resolved
@dmnapolitano
Copy link
Contributor Author

This looks v good! Though atm the integration tests are failing. Also when I run a historical election all subunits are unexpected.

Thanks! Sorry about the failing integration tests. I have a question about that in a comment for you (above) because I'm confused about how we're expecting that test to work 🤔

In regards to the unexpected subunits, can you provide an example for that that I can look into? 🤔

@lennybronner
Copy link
Collaborator

This looks v good! Though atm the integration tests are failing. Also when I run a historical election all subunits are unexpected.

Thanks! Sorry about the failing integration tests. I have a question about that in a comment for you (above) because I'm confused about how we're expecting that test to work 🤔

In regards to the unexpected subunits, can you provide an example for that that I can look into? 🤔

oh oops, sorry I missed that. How was this test passing earlier? 🤔

Here is an example of all units being unexpected:

elexmodel 2020-11-03_USA_G --office_id=P --estimands=party_vote_share_dem --geographic_unit_type=county --pi_method=nonparametric --percent_reporting=30 --aggregates=postal_code --historical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants