Circular Dependency for DIPY ?

skoudoro commented 1 year ago

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 commented 1 year ago

any thought about this issue @frheault @arokem ?

arokem commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

arokem commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

skoudoro commented 1 year ago

Hi @frheault,

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

Thanks!

yurivict commented 7 months ago

Any progress in integrating trx into dipy?

skoudoro commented 7 months ago

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): https://github.com/dipy/dipy/issues/2760

tee-ar-ex / trx-python

Circular Dependency for DIPY ? #51