search

tee-ar-ex / trx-python

Python implementation of the TRX file format
https://tee-ar-ex.github.io/trx-python/
BSD 2-Clause "Simplified" License
20 stars 16 forks source link

Trx integration to Dipy (part 2) #59

Closed frheault closed 9 months ago

frheault commented 1 year ago

@arokem @skoudoro Great work!

This really helps, and I think this pyproject file will help Scilpy a lot since we are constantly bleeding edge! I know you just release 1.7, but if you end up doing a 1.7.1 for some reason, that would be an amazing thing to add for us (Sherbrooke people)

skoudoro commented 1 year ago

there is a PR for that: https://github.com/dipy/dipy/pull/2715

the pyproject.toml need to be much more complex than that. it needs to allow build the project, allow pip install -e . , etc....

It is part of the long todolist but priority high. it needs to be done before september and the release of python 3.12. DIPY will not work with python 3.12, with the current state.

frheault commented 9 months ago

@arokem @skoudoro I made some change after testing on my personal Windows 11 Machine, not sure if that was the source of it.

What is the best way to test that again? We merge this PR and 'release' a version 0.1.3? (Then update the Dipy PR with this new version?

arokem commented 9 months ago

I think that we should add a CI with Windows on it, and then if that passes, we can merge that, make a release and then pin the new version as a dependency on DIPY.

BTW, the documentation build failure is because of some incompatibilities in the sphinx-autoapi functionality. We figured this out on pyAFQ, and that can be resolved by pinning in the setup.cfg, as in https://github.com/yeatmanlab/pyAFQ/pull/1038/files.

skoudoro commented 9 months ago

I think that we should add a CI with Windows on it, and then if that passes, we can merge that, make a release and then pin the new version as a dependency on DIPY.

+1 👍

frheault commented 9 months ago

I will finish this up during MICCAI! Thanks!

frheault commented 9 months ago

@arokem Maybe this is not the exact error you had? I tried leaving the sphinx version and removing it and astroid == (or <=) and it is not working.

Maybe I misunderstood the "fix"?

arokem commented 9 months ago

No, I think that you are doing all the right things, but it looks like the current CI error maybe originates from my merge earlier today of #58, so please revert that change first (sorry - I should have just closed that one instead, but it's just a couple of lines, so shouldn't be hard to undo).

frheault commented 9 months ago

No, I think that you are doing all the right things, but it looks like the current CI error maybe originates from my merge earlier today of #58, so please revert that change first (sorry - I should have just closed that one instead, but it's just a couple of lines, so shouldn't be hard to undo).

Ahhh thank you, I was so focused on the few lines on this PR and I did not see that PR, I was puzzled, thank you!

arokem commented 9 months ago

OK - great! Thanks for adding the tests on Mac and Windows. I think this is good to go, so I'll merge it. Let me know if you think we need any other folllow-up. Otherwise, we can make a release after this is merged.