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

Introduce the ability to assign the id variable universally #42

Merged

Conversation

m-visintini
Copy link
Member

User can select id variable in the sidebar in the check selection tab.

HFCS cannot be run until ID variable is selected.

ID variable field was removed from Duplicate and Outlier checks cards.

ID variable is downloaded as csv with the rest of the parameters and can be reimported as per pre-existing behavior.

Updated all info tooltips to match new UX.

User can select id variable in the sidebar in the check selection tab.

HFCS cannot be run until ID variable is selected.

ID variable field was removed from Duplicate and Outlier checks cards.

ID variable is downloaded as csv with the rest of the parameters and can be reimported as per pre-existing behavior.

Updated all info tooltips to match new UX.
@m-visintini m-visintini linked an issue Oct 28, 2024 that may be closed by this pull request
Copy link
Member

@mariarrt94 mariarrt94 left a comment

Choose a reason for hiding this comment

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

@m-visintini This is great! I only have one comment on this particular PR regarding the unit of observation:

  1. The unit of observation is not automatically programmed as hhid, which I think is fine since the unit of observation isn’t always the same as hhid. However, we should include a note in the instructions (using the question mark sign) explaining what this variable is and mentioning that it could be hhid.

  2. For duplicate values, the hhid is no longer needed in the selection of duplicate IDs when downloading and uploading parameters (using that feature). As a result, the duplicates check isn’t automatically enabled since only hhid is required. If you don't select anything additional and only use the default, this check won’t be triggered. This isn’t a problem, but it would be helpful to mention it somehow in the instructions. This is more of a note for me.

  3. Not related to this feature. While reviewing, I noticed some bugs related to downloading and uploading parameters. Specifically, the selection of the IQR/SD method isn’t being tracked correctly.

Apart from these points, everything looks good, and I think we can proceed to merge main into this PR and then this PR back into main (or follow whichever conflict resolution method you prefer).

Let me know if it would be useful to chat. Great work—thanks again!

@m-visintini
Copy link
Member Author

m-visintini commented Oct 28, 2024

Thank you!

A couple of follow up questions:

  • For 1.: can add a note for now, however I think we need to review the Unit of Observation check as a whole. At the moment, it does not provide any summarization - it just shows the entire dataset. Maybe we can have a discussion about this. I can add a comment now anyways for completeness.
  • For 2.: The current implemented behavior is: (1) Duplicate check is selected by default, but RUN button is disabled (2) when the user selects an ID variable, the RUN button is enabled. If I click it without changing anything else, HFCS runs the duplicate check on ID. What do you suggest as intended behavior?
  • For 3.: Looks like method and multiplier were never included in the export to begin with. I will create a separate issue.

I will merge main into this branch as soon as I have fixed these things, and then commit again!

Unit of observation instruction notes + cleanup
@m-visintini m-visintini merged commit 04ddfd8 into main Oct 31, 2024
@m-visintini m-visintini deleted the 24-introduce-the-ability-to-assign-id-variable-universally branch October 31, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce the ability to assign ID variable universally
2 participants