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

Add file, file_parameter and scanner SQLAlchemy models #1207

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Nov 11, 2024

In order to improve the integration tests for run_dicom_archive_loader.py. I want to query the files table to make sure the files are inserted in the database. This PR adds SQLAlchemy model definitions for this table (and parameter_file and mri_scanner because why not).

The integration tests pass therefore the models are correct !

@maximemulder maximemulder marked this pull request as ready for review November 11, 2024 18:51
@maximemulder
Copy link
Contributor Author

Just one quick question. I named the model for the files table mri_file.py. Is this name correct ? Or can there be files that are not MRI @cmadjar ?

@maximemulder maximemulder changed the title Add file and file_parameter tables Add file, file_parameter and scanner SQLAlchemy models Nov 11, 2024
@cmadjar
Copy link
Collaborator

cmadjar commented Nov 11, 2024

@maximemulder there can be files in the files table that are not MRI. I would change to imaging_file.py instead so that any imaging modality is covered (PET, MRI, MRS etc..)

@maximemulder maximemulder marked this pull request as draft November 12, 2024 14:36
@maximemulder
Copy link
Contributor Author

maximemulder commented Nov 12, 2024

Ahok makes sense. Do you prefer just file.py or imaging_file.py ? The latter seems a little long to me but if you prefer it I'll use it instead. I could also use img_file.py but I don't think we use img as a shorthand of imaging anywhere else 🤔

@cmadjar
Copy link
Collaborator

cmadjar commented Nov 12, 2024

I think just file.py makes sense

@maximemulder
Copy link
Contributor Author

Okay, I changed the name ! By the way I used file_parameter.py instead of parameter_file.py because it makes more sense to group the parameters of a file it with the files than with the parameter types to me. But this is a weak opinion, if you think parameter_file.py is a better name that's fine by me.

@maximemulder maximemulder marked this pull request as ready for review November 12, 2024 21:47
@cmadjar
Copy link
Collaborator

cmadjar commented Nov 12, 2024

Probably best to stick with parameter_file like the table name to avoid confusions.

@maximemulder
Copy link
Contributor Author

maximemulder commented Nov 13, 2024

Updated ! Once this is merged I will update the integration test PR to also ensure the files have been inserted in the database (hopefully Friday).

Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

LGTM

@cmadjar cmadjar merged commit 7d87ea9 into aces:main Nov 13, 2024
9 checks passed
@cmadjar cmadjar added this to the 27.0 milestone Jan 8, 2025
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.

2 participants