Closed manuegrx closed 11 months ago
The fix seems ok. Wouldn't it be possible to have a Bvec type for bvec files and a Bvac type for bvac files rather than a common type (Bvec_Bval) for both? Currently, the unit tests are falling. Some are still in progress. Maybe they'll work. Se need that they work before merging.
I added one type for bevc and one type for bval !
The test are currently failing because the "Bvec_Bval" tag is not in the log export json from mriconv for the test data :
======================================================================
ERROR: test_import_data (__main__.TestMIAMainWindow)
Opens a project and simulates importing a file from the mri_conv
----------------------------------------------------------------------
Traceback (most recent call last):
File "./populse_mia/test.py", line 4461, in test_import_data
self.main_window.import_data()
File "/Users/appveyor/projects/populse-mia/populse_mia/user_interface/main_window.py", line 797, in import_data
new_scans = data_loader.read_log(self.project, self)
File "/Users/appveyor/projects/populse-mia/populse_mia/data_manager/data_loader.py", line 663, in read_log
main_window.progress.exec()
File "./populse_mia/test.py", line 4453, in <lambda>
ImportProgress.exec = lambda self_, *args: self_.worker.run()
File "/Users/appveyor/projects/populse-mia/populse_mia/data_manager/data_loader.py", line 407, in run
if dict_log["Bvec_bval"] == "yes":
KeyError: 'Bvec_bval'
----------------------------------------------------------------------
I will check tomorrow how to fix this
Cool for the bval / bvec!
Oh I see for the UT!!!!!! We need to update the data for the UT. The current ones don't actually integrate the Bvec_bval tag in the transfer json!
Patch coverage is 5.88%
of modified lines.
Files Changed | Coverage |
---|---|
populse_mia/test.py | ø |
populse_mia/data_manager/data_loader.py | 1.53% |
populse_mia/data_manager/project.py | 100.00% |
:loudspeaker: Thoughts on this report? Let us know!.
I added the indexation in the database for bvec/bval in MRTRIX format.
The way I see it we will have :
if BIDS used for import --> only bvec/bval in FSL format (it is mandatory in BIDS to have bvec/bval in FSL format )
if DICOM used for import --> bvec/bval in FSL format and bvec/bval in MRTRIX format. We need to be sure about MRTRIX and FSL format provided by mri conv (see https://github.com/populse/mri_conv/issues/14). Moreover it is only working for Brucker and Philips scanner.
if PAR/REC used for import --> bvec/bval in FSL format and bvec/bval in MRTRIX format. We also need to check format consistency in mri_conv
For the 3 cases, the bvec/bval provided will be correctly indexed in the database.
Even if we need to do some check in mri_conv about FSL and MRTRIx format I thing we can already merge this PR if it is okay for you @servoz
For me, it's OK. Just waiting for the UTs to finish before merging.
We regularly lose UT coverage. This isn't too serious, as we're still above 85%. We'll have to think about completing the UTs when we have a bit of time.
In order to index bvec and bval file in the database, I modified a little bit the run method of ImportWorker.
I added a new "type" (
TYPE_BVEC_BVAL
) for bvec/bval file and I aslo added a new tag (AssociatedNIfTIFile
) in order to be sure to keep a link between the bvec/bval file and the nifti@servoz let me know if it seems okay for you
It is working with BIDS format as input, I need to test with Nifti, DICOM and par/rec