Skip to content

Conversation

@MichaelStritt
Copy link
Contributor

Linked issue

Check out #799

How to test

Required: if not defined in the linked issue, add a simple test description here

Comments

Optional: add helpful comments for the reviewers here

@MichaelStritt MichaelStritt added the import Related to data import module label Oct 4, 2021
@HenkMutsaerts
Copy link
Member

@MichaelStritt @jan-petr Why do we want the temp folder in the derivatives? Temp should in fact be rawdata. derivatives is the output of a processing pipeline, not of a dcm2bids conversion. Let's keep temp as it is. If we need to change anything to move the import module to a full module, we should remove the temp step and integrate it within rawdata. Let's discuss next week.

Copy link
Member

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

Let's wait with this and discuss properly, there are easier/less custom ways to do this, it seems.

@jan-petr jan-petr linked an issue Oct 5, 2021 that may be closed by this pull request
@jan-petr
Copy link
Contributor

jan-petr commented Oct 5, 2021

@MichaelStritt @jan-petr Why do we want the temp folder in the derivatives? Temp should in fact be rawdata. derivatives is the output of a processing pipeline, not of a dcm2bids conversion. Let's keep temp as it is. If we need to change anything to move the import module to a full module, we should remove the temp step and integrate it within rawdata. Let's discuss next week.

We've actually discussed this already on Monday. rawdata is BIDS and thus shouldn't contain any temp or lock files. So the only logical place to put those two is in derivatives because we don't want them hanging around elsewhere. We can of course move temp to rawdata as this is created only for a brief moment... So we can have this discussion on Monday, but it would be great to agree on keeping lock in derivatives so that Michael can proceed with the lock-files.

@jan-petr jan-petr self-requested a review October 5, 2021 07:32
@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt
Copy link
Contributor Author

@jan-petr: The funny thing right now is that it crashes a few times and you get a nice amount of warnings, but the results are still fine. Let me explain. Right now numOf = x.modules.import.numOf; in xASL_imp_DCM2NII_Subject is not updated correctly. I saw warnings pop up if we have two visits for the first but only a single visit for the second subject. This issue is somewhat related to the way that xASL_Iteration runs the pipeline, too. Right now it tries to execute xASL_imp_NII2BIDS once for every subject, because we run xASL_module_Import for each subject, but xASL_imp_NII2BIDS itself iterates over the subjects, which is why the workflow crashes. After the first run of the Import module we already have the correct results though, so the crash happens but the correct results are there. We probably need to move the iteration over the subjects away from the import functions and do that using xASL_Iteration, but then we also need to make sure that things like deleting temp and so on only happen after everything is done, so that has to go to ExploreASL_ImportMaster. A more or less problematic thing remains. In xASL_imp_NII2BIDS we iterate over SubjectSessions, so the combined thing. We have to iterate over sessions only if we run the import module for each subject individually. For DCM2NII it's working without any crashes, but still not correct. There the workflow creates all the temp directories and files and after that it tries to do it again but sees that they are there already and skips it. Again, this happens because we run the import module for each subject. This is the way xASL_Iteration was written though and probably what you guys need to run larger studies. Could be possible that I need some help with that stuff, because it seems that with this request we opened pandoras box...

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Minor changes only. Additionally - I don't want to add the lock files and log files to derivativesReference (because these can potentially change and we don't really need a reference for these). With new derivativesReferences, I will not add it to Flavors, but we need to make sure that the compare-differences ignores them.

I am now running the full-flavor test to see if all works.

@MichaelStritt

This comment has been minimized.

@jan-petr
Copy link
Contributor

Testing

@jan-petr: I just retested. Seems to work just fine on windows, but maybe I need to change filesep to backslash in the last commit for linux support. The differences below are there because I removed GeneratedBy from dataPar, because we wanted that anyway, right?

>> load('...\ExploreASL\Testing\results.mat')
>> flavors.comparisonTable

ans =

  3×4 table

                 flavor                  dataset              name                                   message                      
    _________________________________    _______    ________________________    __________________________________________________

    'GE_PCASL_3Dspiral_14.0LX_1'         'Both'     'Different file content'    'Different file content: \ExploreASL\dataPar.json'
    'Philips_PCASL_3DGRASE_5.4.0.2_1'    'Both'     'Different file content'    'Different file content: \ExploreASL\dataPar.json'
    'Siemens_PCASL_3DGRASE_VE11C_2'      'Both'     'Different file content'    'Different file content: \ExploreASL\dataPar.json'

>> flavors.loggingTable

ans =

  0×3 empty table

Don't use filesep, but fullfile - that's OS non-specific...

also - Philips_PCASL_2DEPI_PARREC_1 did create temp-files, but never rawdata.

Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Philips_PCASL_2DEPI_PARREC_1 doesn't work - rawdata not created and subsequently, the comparison script crashes.

@MichaelStritt
Copy link
Contributor Author

Philips_PCASL_2DEPI_PARREC_1 doesn't work - rawdata not created and subsequently, the comparison script crashes.

@jan-petr: the problem was that I seemingly added a superfluous underscore because of the _1 in the tokens. It should work now. I also fixed the escaping, which should basically only be necessary for windows anyway. af1e66b 👍

@jan-petr

This comment has been minimized.

@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Oct 17, 2021

It still seems to complain about Lock-files in the comparison. And it displays a difference in Generated by, while for this purpose, it should ignore it - that's important for import and processing to note version differences, but not for Flavor reference:

Yeah, wasn't my intention to leave that in there. There's probably just a small mistake somewhere where I do the regular expressions. I'll give it a quick try on the AMC server.

Edit: Oh, okay, so this goes wrong for Koens Hadamard dataset? Is the import already working for that one?

Edit 2: @jan-petr better after this d37c17c & e118d58 ? I just did a stupid mistake with the escaping it seems...

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@MichaelStritt

This comment has been minimized.

@jan-petr
Copy link
Contributor

jan-petr commented Oct 27, 2021

Known issues:

  1. 7T dataset: BIDS->Legacy is almost good, but it copies only ASL and not M0 - error is in xASL_bids_BIDS2Legacy_ParseModality, where in the second iteration of the first loop starts looking for m0scan, but doesn't find them as the Reference varaible still contains ASL and wasn't updated - probably things went wrong when reorganizing the code...
  2. GE_PCASL_3Dspiral_25.0LX_ADNI3_027_S_5079_1 - issue with visits 👍 6d44934
  3. Philips_PASL_2DEPI_5.4.1.0_ADNI3_006_S_6681_1 - issue with visits 👍 6d44934
  4. Philips_PCASL_2DEPI_PARREC_1
    1. I think something is wrong here related to xASL_imp_AddVisitNames within xASL_imp_DetermineStructureFromSourcedata, because the visit is added as _1 instead of ASL_1
    2. visit is added as "1" and not as "_1" - creating "sub-011" instead of "sub-01_1",
    3. also - issues with copying everything 'perf' stuff to BIDS.
    4. One minor bug fixed 7f40cd4. 👍
  5. Legacy comparison should skip the following (shortened):
    Dataset: derivativesReference
    Missing: /ExploreASL/lock
    Missing: /ExploreASL/lock/xASL_module_Import/Sub103/xASL_module_Import/999_ready.status
    Missing: /ExploreASL/log/import_summary_Sub103.csv
    Missing: /ExploreASL/log/xASL_module_Import_Sub103.log
    Missing: /ExploreASL/sub-Sub103_1/bids_report_sub-Sub103.json
    File: /ExploreASL/dataPar.json
    Extra field: GeneratedBy
    Different value: x (array)

MichaelStritt and others added 27 commits November 15, 2021 15:59
@MichaelStritt MichaelStritt merged commit 35745b9 into develop Nov 15, 2021
@MichaelStritt MichaelStritt deleted the import-#799_Module branch November 15, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

import Related to data import module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make import a full module

4 participants