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

[GEN-1313] Export detailed columns for NAACCR codes #567

Merged
merged 10 commits into from
May 28, 2024

Conversation

danlu1
Copy link
Contributor

@danlu1 danlu1 commented May 17, 2024

Problem:

Need to export detailed columns for NAACCR codes

Solution:

Add the detailed columns when corresponding race, sex, and ethnicity columns are available.

Test:

Tested locally and ran through validation and processing steps successfully with expected results.

@danlu1 danlu1 requested a review from a team as a code owner May 17, 2024 17:10
Copy link
Contributor

@rxu17 rxu17 left a comment

Choose a reason for hiding this comment

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

Code looks good so far! i would just adjust the tests so they pass

@danlu1 danlu1 closed this May 20, 2024
@danlu1
Copy link
Contributor Author

danlu1 commented May 20, 2024

Close pr for now to rebuild docker image

@danlu1 danlu1 reopened this May 28, 2024
Copy link
Contributor

@rxu17 rxu17 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work! Just a couple of comments but going to pre-approve. Also just a heads up, i think you have to merge in changes from develop

tests/test_clinical.py Outdated Show resolved Hide resolved
tests/test_clinical.py Show resolved Hide resolved
Copy link
Contributor

@rxu17 rxu17 May 28, 2024

Choose a reason for hiding this comment

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

Just a comment. Nothing actionable here. I think it's OK we don't have unit tests for these giant functions: stagingToCbio, store_clinical_files and consortiumToPublic in the database_to_staging and consortium_to_public code just because these are better tested using integration tests and we already did that with our pipeline comparisons and our test runs on the test pipeline.

Doing this would be outside the scope of this ticket and is already part of our tech debt epic to refactor and add tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that we add unit tests later for these functions for our tech debt epic.

Copy link
Member

Choose a reason for hiding this comment

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

The more we can spin out "unit functions" within these larger functions, the more they will be better served as integration tests.

If we think of these as ETL, it should tell the story of data processing.

Copy link

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@danlu1 danlu1 merged commit 2f3f2f2 into develop May 28, 2024
14 checks passed
@rxu17 rxu17 deleted the GEN-1313-create-naaccr-codes branch May 29, 2024 07:34
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.

3 participants