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

Summary QC #844

Merged
merged 43 commits into from
Jul 29, 2024
Merged

Summary QC #844

merged 43 commits into from
Jul 29, 2024

Conversation

kenkehoe
Copy link
Contributor

This PR contains a new method to create the Standardized QC Summary file. It will convert current embedded QC which is bit packed to integer/flag QC that contains a single test per assessment. Plan is for ADC Data Discovery to use this code to generate QC Summary files on the fly for user order. This is part of ARM TSK0312159.

@kenkehoe
Copy link
Contributor Author

Don't integrate the PR yet. I need to discuss current implementation with Adam and Michael first.

act/qc/qc_summary.py Outdated Show resolved Hide resolved
act/qc/qc_summary.py Outdated Show resolved Hide resolved
@zssherman
Copy link
Collaborator

@kenkehoe Thanks for the PR! I added some review comments

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Just a couple small suggestions - same as Zach

act/qc/qc_summary.py Outdated Show resolved Hide resolved
act/qc/qc_summary.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

thanks for the quick response @kenkehoe

@zssherman
Copy link
Collaborator

@kenkehoe Cool with me merging this? I saw Adam approved it

@AdamTheisen
Copy link
Collaborator

@kenkehoe looks like some linting/pep8 errors

@zssherman zssherman closed this Jul 24, 2024
@zssherman zssherman reopened this Jul 24, 2024
@zssherman zssherman closed this Jul 24, 2024
@zssherman zssherman reopened this Jul 24, 2024
@zssherman zssherman closed this Jul 24, 2024
@zssherman zssherman reopened this Jul 24, 2024
@zssherman
Copy link
Collaborator

@kenkehoe You cool with me merging this?

@kenkehoe
Copy link
Contributor Author

@zssherman hold off on merger. I found one more issue and I need to run some more testing. Hopefully this will be figured out soon.

@zssherman
Copy link
Collaborator

@kenkehoe No worries at all!

@AdamTheisen
Copy link
Collaborator

@kenkehoe how's this looking for merging?

@zssherman have your requested changes been implemented? Can we resolve that request?

@kenkehoe
Copy link
Contributor Author

kenkehoe commented Jul 29, 2024

@AdamTheisen I believe this ready to merge. I have tested the crap out of this and it is working pretty well. My only issue currently is with the arm-test-data repo when used as a clone vs. PIP install.

@zssherman zssherman merged commit 8ac845a into ARM-DOE:main Jul 29, 2024
19 checks passed
@zssherman
Copy link
Collaborator

@kenkehoe Thanks for doing those changes! Merging

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.

Feature Request: QC function to summarize the QC
4 participants