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

Circular Dependency for DIPY ? #51

Open
skoudoro opened this issue Mar 6, 2023 · 18 comments
Open

Circular Dependency for DIPY ? #51

skoudoro opened this issue Mar 6, 2023 · 18 comments

Comments

@skoudoro
Copy link

skoudoro commented Mar 6, 2023

Hi @frheault, Hi @arokem,

I would like to start integrating TRX in DIPY.

However:

  • I see that some part of TRX depends on DIPY
  • if I add TRX as a DIPY dependency, it looks like we enter in a circular dependency which will create weird issues.

What is the best approach to solve this coming issue ?

  • Integrate TRX in Nibabel ?
  • integrate Stateful Tractogram in Nibabel (this is not the only DIPY element used in this package)
  • Integrate TRX in DIPY ?
  • Other ideas ? @Garyfallidis ?

Thank you for your feedback !

@skoudoro
Copy link
Author

any thought about this issue @frheault @arokem ?

@arokem
Copy link
Collaborator

arokem commented Mar 20, 2023

I think that it would be great if we could make TRX not depend on DIPY. I do think that one good step in that direction would be to integrate the StatefulTractogram object upstream into nibabel. Admittedly, this is not the only DIPY dependency here, but it would take care of a large part of the dependency. The rest could potentially then be migrated as DIPY functionality, or more clearly separated out here.

@frheault
Copy link
Collaborator

frheault commented Mar 20, 2023

As of now, the TRX core does not require Dipy, but the most useful scripts do require Dipy simply because when it comes to manipulating or visualizing real data Dipy or Fury is involved.

The function to_sft() does require Dipy. Once the code is 'mature enough' in TRX I could make sure the StatefulTractogram works with TRX (although SFT does load everything in RAM).

We talked about moving the StatefulTractogram to Nibabel, but from my point of view it is just a 'class' that wrapper around a few loader/writer to make it consistent to use with spatial information.

I am not sure what is the best approach here, but I made a few scripts to make TRX easy to use/test and only these depend on Dipy, we avoided including Dipy in the core, but for a script as complex as 'validate' or 'convert' need the StatefulTractogram is required (because that's a meta-script, it does a lot of check and all).

@frheault
Copy link
Collaborator

I don't think StatefulTractogram goes in Nibabel, its operation are really high-level (consistency between files, validity check mostly specific to Dipy, functions useful for other Dipy usage), while I feel like Nibabel is really about file format (header, data, basic checks).

If only moving the 'core', TRX could move to Nibabel, but we would still need a repo full of scripts that mix Nibabel (to get to the TRX) and Dipy (to get to the StatefulTractogram) to make it nice and easy to use.

@arokem
Copy link
Collaborator

arokem commented Mar 20, 2023

I think that the day trx "core" migrates to nibabel is still a bit far off. For the time being, we could have two separate repos in the tee-ar-ex org that have "core" functionality and "nice-to-have" (i.e., depend on DIPY/FURY) separated between them.

@skoudoro
Copy link
Author

I like this last idea. Let me know if you need help to start this process

@arokem
Copy link
Collaborator

arokem commented Mar 20, 2023

Hey @skoudoro : how would you feel about moving functionality that depends on DIPY into DIPY itself? For example, a convert_tractogram workflow that would convert between formats, including trx, a concatenate_tractogram workflow, validate_tractogram, etc.

These are generally useful, and since they already require DIPY, anyone using them would need to have DIPY anyway, and they are not trx-specific, because they also work on other file formats.

@skoudoro
Copy link
Author

I am really positive, and I think it would be great if they could be merged for the future release before the workshop.

Can I start this task? I might be able to work on it this Friday?

or you want to do it @frheault ? @arokem ?

@arokem
Copy link
Collaborator

arokem commented Mar 22, 2023

I'd be up for a sprint on this on Friday afternoon (2-4PM PT, which is 5-7 EST), if you folks are up for it. Alternatively, if that timing is rotten for you, put up a PR earlier in the day and I will review it in my afternoon.

@skoudoro
Copy link
Author

sound like a plan. I will do my my best to be online during this time slot. I might start on the EST morning

@frheault
Copy link
Collaborator

I will be sure to start working on this friday morning, I can sync with @skoudoro (maybe a quick call to have a plan) and work an extra hour or two in the afternoon and 'debrief' @arokem so you can be updated (because I don't think I will work that late on a Friday).

@arokem
Copy link
Collaborator

arokem commented Mar 22, 2023

OK. I sent you both a calendar invite with a zoom link. If anyone else wants to join for this session, just let me know and I will add you as well.

@skoudoro
Copy link
Author

thanks @arokem !

  • ok @frheault, I might be available at 10am Est, if not, for sure at 11am. Just send me a link and I will keep you updated for the time

@frheault
Copy link
Collaborator

I had to solve something this morning and I have the lab meeting in 45 minutes. So I will work on this in the afternoon after all.

@skoudoro
Copy link
Author

we were talking about Friday morning @frheault, I am not available today, quite busy

@skoudoro
Copy link
Author

skoudoro commented Mar 24, 2023

Hi @frheault,

I am available from now until 1pm, Please send me email with a zoom/google link, whenever you are ready.

Thanks!

@yurivict
Copy link

Any progress in integrating trx into dipy?

@skoudoro
Copy link
Author

Hi @yurivict,

on the last DIPY release, the base of TRX is integrated in DIPY. So currently, DIPY depends on TRX.

Next steps for next release (probably in march): dipy/dipy#2760

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

4 participants