Mic-4566/omit-row-update #352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
albrja
commented
Nov 27, 2023
| DATASETS.wic.name: { | ||
| Keys.ROW_NOISE: { | ||
| NOISE_TYPES.omit_row.name: { | ||
| Keys.ROW_PROBABILITY: 0.005, |
There was a problem hiding this comment.
Default value of 0 has been updated for WIC
albrja
commented
Nov 27, 2023
| @@ -468,19 +468,6 @@ def test_get_config(caplog): | |||
| assert column_noise_dict[column_noise][Keys.CELL_PROBABILITY] == 0.0 | |||
|
|
|||
|
|
|||
| def test_omit_rows_do_not_respond_mutex_default_configuration(): | |||
There was a problem hiding this comment.
This test is no longer true as datasets can have both omission types.
tests/integration/test_interface.py
Outdated
| if dataset_name in [DATASETS.census.name, DATASETS.acs.name, DATASETS.cps.name]: | ||
| # Census and household surveys has no do_not_respond and omit_row. | ||
| # For all other datasets they are mutually exclusive | ||
| assert len(noise_type) <= 2 |
There was a problem hiding this comment.
I'm confused. it looks like census and surveys already have do_not_respond and you just added omit_row. Your comment says the opposite. And I think it should be assert len(noise_type) == 2
There was a problem hiding this comment.
You are right yes it should be 2.
albrja
requested review from
hussain-jafari,
mattkappel,
ramittal and
rmudambi
as code owners
mattkappel
approved these changes
Nov 27, 2023
| @@ -273,7 +273,7 @@ def test_row_noising_omit_row_or_do_not_respond(dataset_name: str, config, reque | |||
| if dataset_name in [DATASETS.census.name, DATASETS.acs.name, DATASETS.cps.name]: | |||
| # Census and household surveys has no do_not_respond and omit_row. | |||
| # For all other datasets they are mutually exclusive | |||
| assert len(noise_type) <= 2 | |||
| assert len(noise_type) == 2 | |||
There was a problem hiding this comment.
Fix the comment on line 274
stevebachmeier
approved these changes
Nov 28, 2023
rmudambi
pushed a commit
that referenced
this pull request
Jan 19, 2024
Mic-4566/omit-row-update Add omit row to census and household surveys - *Category*: Feature - *JIRA issue*: [MIC-4566](https://jira.ihme.washington.edu/browse/MIC-4566) Updates -adds omit row to census and household surveys (ACS and CPS) -> docs: https://vivarium-research.readthedocs.io/en/latest/models/concept_models/vivarium_census_synthdata/concept_model.html#row-noise Testing -Updated tests because do not respond and omit row are no logner mutually exclusive. All tests pass
albrja
added a commit
that referenced
this pull request
Feb 12, 2024
Mic-4566/omit-row-update Add omit row to census and household surveys - *Category*: Feature - *JIRA issue*: [MIC-4566](https://jira.ihme.washington.edu/browse/MIC-4566) Updates -adds omit row to census and household surveys (ACS and CPS) -> docs: https://vivarium-research.readthedocs.io/en/latest/models/concept_models/vivarium_census_synthdata/concept_model.html#row-noise Testing -Updated tests because do not respond and omit row are no logner mutually exclusive. All tests pass
albrja
added a commit
that referenced
this pull request
Feb 12, 2024
Mic-4566/omit-row-update Add omit row to census and household surveys - *Category*: Feature - *JIRA issue*: [MIC-4566](https://jira.ihme.washington.edu/browse/MIC-4566) Updates -adds omit row to census and household surveys (ACS and CPS) -> docs: https://vivarium-research.readthedocs.io/en/latest/models/concept_models/vivarium_census_synthdata/concept_model.html#row-noise Testing -Updated tests because do not respond and omit row are no logner mutually exclusive. All tests pass
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mic-4566/omit-row-update
Add omit row to census and household surveys
-adds omit row to census and household surveys (ACS and CPS) -> docs: https://vivarium-research.readthedocs.io/en/latest/models/concept_models/vivarium_census_synthdata/concept_model.html#row-noise
Testing
Updated tests because do not respond and omit row are no logner mutually exclusive. All tests pass