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

[FIX] Address several PET bug issues #110

Merged
merged 14 commits into from
Feb 6, 2024
Merged

[FIX] Address several PET bug issues #110

merged 14 commits into from
Feb 6, 2024

Conversation

dlevitas
Copy link
Collaborator

@dlevitas dlevitas commented Jan 29, 2024

Further addresses #92

This PR address several issues pertaining to PET BIDS conversion:

1). Detect pet/blood data (both json and corresponding tsv files) and separate metadata requirements from pet/pet data.

2). Convert pet/blood data into a BIDS-compliant dataset.
3). Accepts ECAT-formatted data (.v, .v.gz)
4). Utilizes parallel for file renaming (rare, but but otherwise incredibly slow when renaming thousands of files)

@dlevitas
Copy link
Collaborator Author

@bendhouseart would you mind reviewing the functionality introduced in this PR?

@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 29, 2024

Checked the following on my end:

setup success status msg/error ID
Phantom with the example spreadsheet fail failed to run bids -- code:1 /app/handler/convert.js:5 import { readFileSync, writeFileSync, openSync, writeSync, closeSync, lstatSync, unlinkSync, linkSync } from 'fs'; ^^^^^^ SyntaxError: Cannot use import statement outside a module at Object.compileFunction (node:vm:360:18) at wrapSafe (node:internal/modules/cjs/loader:1126:15) at Module._compile (node:internal/modules/cjs/loader:1162:27) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10) at Module.load (node:internal/modules/cjs/loader:1076:32) at Function.Module._load (node:internal/modules/cjs/loader:911:12) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) at node:internal/main/run_main_module:22:47 #65b830fb4c1a13a89093658e
Phantom with an "incorrect" spreadsheet, then correcting in metadata editor fail "status_msg": "failed to run bids -- code:1\n/app/handler/convert.js:5\nimport { readFileSync, writeFileSync, openSync, writeSync, closeSync, lstatSync, unlinkSync, linkSync } from 'fs';\n^^^^^^\n\nSyntaxError: Cannot use import statement outside a module\n at Object.compileFunction (node:vm:360 #65b82b354c1a13a890936463
Real data from one of our COX-2 Subjects fail timed out, forced quit #65b82e574c1a13a8909364ee

Not complete until each row is filled out*

@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 29, 2024

The real data seems to be getting stuck somewhere, it's been running several minutes and this is the state of the output dir:

preprocess.err	preprocess.log	sub-0

Could be the pax headers messing things up:

.
└── sub-09
    ├── anat
    └── pet
        ├── ses-01
        │   └── PaxHeaders.29559
        └── ses-02
            └── PaxHeaders.2255

or the fact that dicoms in ses-01 and ses-02 have the following format:

LASTNAME_FIRSTNAME.PT.PET_PET_BRAIN_DYN_TRACER_34MA_(ADULT).0004.4664.2020.06.28.12.23.29_

@dlevitas
Copy link
Collaborator Author

Are there any errors in the web browser console?

@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 30, 2024

Only at the finalize page see:
Screenshot 2024-01-29 at 7 35 47 PM

That said, we can look over the sessions in the /tmp/ezbids-workdir together if you like.

Updated table above, will play again tomorrow and try out some conversions without blood data/spreadsheets.

@dlevitas
Copy link
Collaborator Author

dlevitas commented Jan 30, 2024

Just pushed some commits that should resolve the finalize page error

@dlevitas dlevitas removed the request for review from bhatiadheeraj January 30, 2024 01:06
@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 30, 2024

Take #2 following changes added with commit 8ad5b63

setup success status msg/error ID
Phantom with the example spreadsheet PASS None #65b952b02e56eca23622cfc4
Phantom with an "incorrect" spreadsheet, then correcting in metadata editor PASS No complaints #65b9695805fb3d4fbcac9a5e
Real data from one of our COX-2 Subjects TIMEOUT still timing out, upload succeeds, but gets stuck on the inflating step 65b95b2f2e56eca23622d0a9

Not complete until each row is filled out*

@bendhouseart
Copy link
Contributor

Screenshot 2024-01-30 at 2 52 58 PM

@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 30, 2024

One thing I did notice that the metadata menu/editor doesn't agree with the value in the spreadsheet. But it's reading in everything else correctly and producing an invalid output (that's what I wanted to see) as a result.

Screenshot 2024-01-30 at 2 59 11 PM Screenshot 2024-01-30 at 3 00 12 PM

@dlevitas
Copy link
Collaborator Author

dlevitas commented Jan 30, 2024

One thing I did notice that the metadata menu/editor doesn't agree with the value in the spreadsheet. But it's reading in everything else correctly and producing an invalid output (that's what I wanted to see) as a result.

This is by design for MetaboliteRecoverCorrectionApplied. If that value is True and the corresponding tsv header column ("hplc_recovery_fractions") doesn't exist, ezBIDS changes the value to False. It's attempting to correct a mistake that, if left unchecked, would result in a bids-validator error later on.

@bendhouseart
Copy link
Contributor

bendhouseart commented Jan 30, 2024

Got it, so I just tried to rerun the data that produced the above screenshots, but that when I do ezBIDS doesn't pick up on the petness of the files and run dcm2niix instead.

Screenshot 2024-01-30 at 3 19 27 PM

Which is weird.

@dlevitas
Copy link
Collaborator Author

dlevitas commented Jan 30, 2024

Got it, so I just tried to rerun the data that produced the above screenshots, but that when I do ezBIDS doesn't pick up on the petness of the files and run dcm2niix instead.

You mean you uploaded the output from your first attempt, meaning a BIDS-compliant dataset, or something else? I'm a little unsure how to re-produce this issue.

@bendhouseart
Copy link
Contributor

I reran it on the same data in the same folder. Then I made a duplicate of that data, and it produced the same error.

What I haven't done is to drag and drop data, I've been using the file browser instead.

@bendhouseart
Copy link
Contributor

Also, I think the issue with it not picking up the pet files was because I had excel open, I would call this good to go for now and merge it.

@dlevitas
Copy link
Collaborator Author

dlevitas commented Jan 30, 2024

I'm going to perform a few additional tests, but I'll plan to merge this PR by the end of the week.

@dlevitas
Copy link
Collaborator Author

dlevitas commented Feb 1, 2024

@bendhouseart when you get the chance, would you mind looking over commit 64043f3?

I ended up adding ECAT support (via ecatpet2bids) and merged the functionality of find_dicomdir.py and find_petdir.py into the former and removed the latter. To test this, I uploaded a "dataset" containing 5 different types of data:

  • MRI dicoms
  • MRI NIfTI/JSON files
  • PET dicoms
  • PET ECAT-formatted files (.v)
  • PET NIFTI/JSON (and blood recording) files, transformed previously with pypet2bids.

All files were successfully identified and transformed based on their datatype and format (i.e. MRI dicoms --> dcm2niix; PET dicoms --> dcm2niix4pet; PET ECAT --> ecatpet2bids).

The PET data I was able to find online is still rather small, so I haven't replicated your inflation error from before.

@bendhouseart
Copy link
Contributor

bendhouseart commented Feb 6, 2024

Just tested the final (it's absolutely final i'm sure) iteration you pushed yesterday and it works flawlessly with our real data.

Screenshot 2024-02-06 at 11 07 55 AM

So long as users place the extra PET metadata spreadsheet along with the dicom files things go off without a hitch. One might argue that ezBIDS should be able to locate a PET metadata spreadsheet from anywhere, but I would call this done as the user has the interface to correct for missing metadata.

@dlevitas
Copy link
Collaborator Author

dlevitas commented Feb 6, 2024

@bendhouseart I recall us discussing how ezBIDS should handle PET metadata spreadsheet(s), landing on users needing to place a spreadsheet in each corresponding raw data folder.

This approach is fine by me; however, I'm also amenable to a provenance framework (akin to BIDS) where all raw data folders or files underneath a spreadsheet are assumed to have that specified metadata. As an example:

-- pet_metadata_spreadsheet.xlsx
-- sub1_raw_data
---- run1_ecat.v
---- run2_ecat.v

Where, both ECAT-formatted raw PET data would be given the metadata in the folder above. If this isn't how PET metadata would be stored, or if it's outside the scope of the PR, we can ignore this.

@bendhouseart
Copy link
Contributor

bendhouseart commented Feb 6, 2024

Out of scope!

What's going to be more helpful to the user, in the immediate, is them being clued in on how to format and place their PET spreadsheets. But, really I don't want to make any assumptions or optimizations until the current iteration gets tested on real users.

@dlevitas dlevitas merged commit 8016bef into master Feb 6, 2024
1 check passed
@dlevitas dlevitas deleted the enh/pet_fixes branch February 6, 2024 16:57
@dlevitas dlevitas mentioned this pull request Feb 6, 2024
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