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

test data failing #81

Open
stebo85 opened this issue Sep 1, 2023 · 2 comments
Open

test data failing #81

stebo85 opened this issue Sep 1, 2023 · 2 comments

Comments

@stebo85
Copy link
Collaborator

stebo85 commented Sep 1, 2023

Dear @rhancockn, @neurosutton, @bpinsard

@Remi-Gau and I are trying to get the HCPPipelines bids-app updated and tested again :)

We are currently running into the problem that the test data is not working, that's what we tried:

          wget https://raw.githubusercontent.com/bids-apps/maintenance-tools/main/utils/get_data_from_osf.sh
          bash get_data_from_osf.sh hcp_example_bids_v3
  
          docker run -ti --rm --read-only \
            -v ~/data/hcp_example_bids_v3:/bids_dataset \
              bids/${CIRCLE_PROJECT_REPONAME,,} \
                /bids_dataset \
                /outputs \
                participant --participant_label 100307 \
                --stages PreFreeSurfer \
                --processing_mode legacy \
                --license_key="*CxjskRdd7" \
                --n_cpus 2

Unfortunately this results in:

Traceback (most recent call last):
  File "/run.py", line 274, in <module>
    t1_spacing = layout.get_metadata(t1ws[0])["DwellTime"]
KeyError: 'DwellTime'

Exited with code exit status 1

Do you by any chance have an idea what this could be?

I also saw a couple of open pull requests. Would anyone of you be able to help us merge these pull requests and get this bidsapp updated and nicely working?

Thank you
Steffen

@neurosutton
Copy link
Contributor

Would changing run.py L274 to t1_spacing = layout.get_metadata(t1ws[0]).get("DwellTime", 0) get most users past the error appropriately? If I understand the comments from #65 correctly, setting 0 as the default here in the get call would skip distortion correction. t1_spacing = layout.get_metadata(t1ws[0]).get("DwellTime", None) might be the better change, if the following comment is the preferred way forward.

@rhancockn Add error handling around t1_spacing = layout.get_metadata(t1ws[0])["DwellTime"] (and maybe t2_spacing = layout.get_metadata(t2ws[0])["DwellTime"]). If distortion correction is to be done, throw a descriptive error message about why this field is required. Otherwise, set t1_spacing = 'NONE'.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Sep 1, 2023

that would probably do it but that would require to rebuild the image which at the moment fails: https://app.circleci.com/pipelines/github/bids-apps/HCPPipelines/30/workflows/f2cad6b5-6f46-4c19-b2a0-71cc32c37b95/jobs/212

so the approach for now was at least make sure that when people pull the latest image from docker hub that it "runs properly" and hence trying to just make sure that original tests pass which apparently is not the case.

I can edit the data on OSF but just want to be sure that this is the right thing to do: I would have assumed that the latest image should work on the dataset that was present when it was first built.

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

No branches or pull requests

3 participants