-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: s3 data validation: setup allure and bugfixes for previous s3 data validation PR bugs #237
Conversation
…i/s3-data-validation-dataset-deposition-photos
…i/s3-data-validation-dataset-deposition-photos
… into daniel-ji/s3-data-validation-frames-gains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just code cleanup here
return tentatives | ||
|
||
|
||
def voxel_spacings(voxel_spacing_files: List[str]) -> List[float]: | ||
def voxel_spacings(voxel_spacing_files: List[str]) -> List[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
voxel spacings are going to be kept as strings, because we want to preserve the folder name (and ensure it's exactly 3 digits of precision)
allure.dynamic.feature(f"Run {item.callspec.params['run_name']}") | ||
if "dataset" in item.fixturenames: | ||
allure.dynamic.epic(f"Dataset {pytest.dataset}") | ||
yield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, all of the past code be just simplified to this, which is doing the same thing 😅
action="store", | ||
default=".", | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleting things that were never used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just renaming *_header functions & fixtures to _metadata, as per discussion with @manasaV3
if len(refined_files) == 0 and len(files) != 0: | ||
pytest.skip(f"No frame files in frames directory: {frames_dir}") | ||
|
||
return refined_files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip checks related to the frames files, if there's no frame files (and only gain files) in the Frames/ folder
841794f
to
079a9ce
Compare
…ted testing for smaller datasets)
@@ -86,7 +77,7 @@ def run_spacing_combinations( | |||
for run in run_names: | |||
for vs in voxel_spacings: | |||
if f"{bucket}/{dataset}/{run}/Tomograms/VoxelSpacing{vs}" in voxel_spacing_files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this could be "{bucket}/{dataset}/{run}/Tomograms/VoxelSpacing{vs:.3f}"
. It would fix the precision at 3 for the float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we could collect everything here (even the non-3f ones) and then in the voxel spacings test, we would confirm this?
setup allure
small fixes
actually modified tests in this PR (not just some name refactors, like many of the modified lines in this PR):