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

[feature proposal] BIDS directory as input #52

Open
neurorepro opened this issue May 15, 2023 · 18 comments
Open

[feature proposal] BIDS directory as input #52

neurorepro opened this issue May 15, 2023 · 18 comments

Comments

@neurorepro
Copy link

neurorepro commented May 15, 2023

Hello,

Would it be of interest to add a feature so that pydeface accepts a BIDS directory as input, to make pydeface potentially run as a BIDS app ?

Many implementations are possible, with one possible first iteration e.g. relying on pybids and involving as parameters:

  • a path to a BIDS directory
  • an optional subject (all by default)
  • an optional suffix (T1w by default)

Best practice seems to be replacing the original file with the defaced one so the output could be:

  • the original non defaced file renamed as ..._desc-nondefaced_..._<suffix>.nii.gz
  • saving the defaced file with the name of the original one (or e.g. ..._desc-defaced_...<suffix>.nii.gz)
@poldrack
Copy link
Contributor

sounds like a great idea to me! probably need to think about how to handle cases where there is already a _desc- flag.

@ofgulban
Copy link
Collaborator

ofgulban commented May 16, 2023

@neurorepro feel free to do a pull request. If you decide to do so, please ensure that your changes do not alter the current default behavior.

@neurorepro
Copy link
Author

@ofgulban Sure, I will work on it and do a PR (everything backward compatible of course !)

@ofgulban
Copy link
Collaborator

saving the defaced file with the name of the original one (or e.g. ..._desc-defaced_...<suffix>.nii.gz)

This seems to have been discussed at #36 , maybe have a look there.

@neurorepro
Copy link
Author

@ofgulban It could be temporarily included in the sidecar json as mentioned in #36.

Also a solution (hopefully soon fully BIDS compatbile) for the case @poldrack mentioned when a desc label is already present is to concatenate values with + as alluded to here

@effigies
Copy link
Contributor

Note that https://github.com/PeerHerholz/BIDSonym wraps pydeface in a BIDS app that allows you to select your defacing method. Seems like it might be unnecessary effort building pydeface out into a BIDS app.

@neurorepro
Copy link
Author

Hi @effigies , yes we saw that repo but it is no longer maintained

@effigies
Copy link
Contributor

Oh. I was under the impression it was. cc @PeerHerholz for clarification.

@neurorepro
Copy link
Author

I think it is also harder to maintain an aggregated tool depending on four defacing utilities than a single defacing utility.

By no longer maintained, i meant no actively maintained (the last commit is from 2021).

@neurorepro
Copy link
Author

If it is deemed unnecessary to add BIDS app functionality to pydeface and @PeerHerholz's BIDSonym is actively maintained, then we will simply use BIDSonym

@PeerHerholz
Copy link

Ahoi hoi folks,

thx @effigies for the tag!

@neurorepro is definitely right in that BIDSonym is not really actively maintained, as I don't have time and funding to do so for a while now. The basic functionality is still working ok (I use the latest version quite often.) but it could for sure need a few updates here and there. I'm of course happy to support respective endeavors!

Cheers, Peer

@neurorepro
Copy link
Author

Thank you @PeerHerholz for your feedback.

Then @effigies do you think it can be useful to add a BIDS app functionality to pydeface ?

@effigies
Copy link
Contributor

Fine by me. I'm not maintaining here...

@ofgulban
Copy link
Collaborator

ofgulban commented May 16, 2023

@neurorepro , looking at the latest commits, it seems that I am the one who is maintaining here. I would be curious to see a PR, but also at the same time worried if it is a big one (conference season is coming with work piling up). So my suggestion would be to maybe if possible start with something small but does what you wanted. I have to say that I am not familiar with the full extend of what "BIDS app functionality" means. I generally have two main worries:

  • Breaking backwards compatibility
  • Increasing the cost of maintenance in the long term (i.e. a lot of added lines of code)

So if you can address these, I would be happy to go through a PR.

@neurorepro
Copy link
Author

@ofgulban sure, BIDS app functionality would mean having it BIDS-aware, i.e. run on a BIDS dataset without having to tell it where is the anatomical data (because BIDS does that)

@ofgulban
Copy link
Collaborator

@neurorepro that sounds cool and useful so that's fine with me.

@Remi-Gau
Copy link

am quickly trying to see if I can build a functional image of bidsonym that would have the latest version of each defacing tool...
just in case this can avoid some wheel reinvention

@Remi-Gau
Copy link

I did not get super far but in case someone has some ideas, I am leaving this in a PR: PeerHerholz/BIDSonym#70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants