Skip to content

Conversation

@jan-petr
Copy link
Contributor

Linked issue

#790

@jan-petr jan-petr requested a review from MichaelStritt August 18, 2021 17:19
@jan-petr jan-petr self-assigned this Aug 18, 2021
@jan-petr jan-petr linked an issue Aug 18, 2021 that may be closed by this pull request
@MichaelStritt
Copy link
Contributor

@jan-petr: will run the flavors tonight and I'll post the results tomorrow.

@jan-petr
Copy link
Contributor Author

@MichaelStritt see my first results

  • Plenty of datasets differ in acknowledgement. Despite the ignoring. My guess is that if both reference and source have the field, then this is ignored, but if only one of the compared sets have it, then it complained about the difference. So it is not about the content but about a difference in existence.
  • Siemens_PCASL_2DEPI_VB17A_BSupN_1 has different asl.nii. By coincidence, this is a dataset from the same center as the one I was adding for the newest tests. My guess is that previously, we were ordering this dataset wrong and this now got fixed :) You can see that in the reference - the motion across repetitions is sketchy. And in the new version, this is much more smooth. Do you agree with this? If yes, then we have to replace the reference asl.nii in the flavors with this newly converted asl.nii
  • The only other issue is with Hadamard - but Beatriz knows about this and is fixing it in some other issue, so we can ignore that.

@MichaelStritt Can you please upload the new asl.nii to flavors and fix the acknowledgement issue in this issue?

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Aug 19, 2021

@jan-petr: here is the print out of the bids comparison: bids_comparison.txt

Plenty of datasets differ in acknowledgement. Despite the ignoring. My guess is that if both reference and source have the field, then this is ignored, but if only one of the compared sets have it, then it complained about the difference. So it is not about the content but about a difference in existence.

I saw the acknowledgement thing. will fix it immediately, should be easy 👍

Fix: 7ef0051

Siemens_PCASL_2DEPI_VB17A_BSupN_1 has different asl.nii. By coincidence, this is a dataset from the same center as the one I was adding for the newest tests. My guess is that previously, we were ordering this dataset wrong and this now got fixed :) You can see that in the reference - the motion across repetitions is sketchy. And in the new version, this is much more smooth. Do you agree with this? If yes, then we have to replace the reference asl.nii in the flavors with this newly converted asl.nii

I saw that too. That's really weird.

Dataset: Siemens_PCASL_2DEPI_VB17A_BSupN_1
File:      \sub-Sub1\perf\sub-Sub1_asl.nii.gz
           RMSE of NIFTIs above threshold.
Warning: RMSE of NIFTIs above threshold: \sub-Sub1\perf\sub-Sub1_asl.nii.gz    
> In xASL_bids_CompareStructures>printDifferencesAsWarnings (line 217)
  In xASL_bids_CompareStructures (line 206)
  In xASL_test_Flavors (line 137) 

I checked out both rawdata NIfTIs in ImageJ and it seems like the new version has less "motion". It really seems like there's something wrong with the ordering in the old one. I personally would say the new one is correct.

Fix: https://github.com/ExploreASL/FlavorDatabase/commit/4f4fd0781440f7c67477b0c8365a846aa3cfaa5d

The only other issue is with Hadamard - but Beatriz knows about this and is fixing it in some other issue, so we can ignore

Agree, we shouldn't fix this here and rather wait for @BeatrizPadrela to fix this 😛

Copy link
Contributor

@MichaelStritt MichaelStritt left a comment

Choose a reason for hiding this comment

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

Should be fine after we fixed the Acknowledgement issue and updated the Siemens dataset 👍

@MichaelStritt MichaelStritt force-pushed the feature-#790_ImportOrderSeriesNumber branch from 7ef0051 to d41053c Compare August 19, 2021 08:13
@MichaelStritt MichaelStritt merged commit d41053c into develop Aug 19, 2021
@MichaelStritt MichaelStritt deleted the feature-#790_ImportOrderSeriesNumber branch August 19, 2021 08:14
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.

Import file ordering by SeriesNumber

3 participants