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

Trx integration to Dipy (part 2) #59

Merged
merged 20 commits into from Oct 7, 2023

Conversation

frheault
Copy link
Collaborator

No description provided.

@frheault frheault changed the title change hash Trx integration to Dipy (part 2) Apr 24, 2023
@frheault
Copy link
Collaborator Author

@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
Copy link

there is a PR for that: dipy/dipy#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
Copy link
Collaborator Author

frheault commented Oct 6, 2023

@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
Copy link
Collaborator

arokem commented Oct 6, 2023

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
Copy link

skoudoro commented Oct 6, 2023

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 👍

Co-authored-by: Ariel Rokem <arokem@gmail.com>
@frheault
Copy link
Collaborator Author

frheault commented Oct 6, 2023

I will finish this up during MICCAI! Thanks!

@frheault
Copy link
Collaborator Author

frheault commented Oct 6, 2023

@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
Copy link
Collaborator

arokem commented Oct 6, 2023

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
Copy link
Collaborator Author

frheault commented Oct 7, 2023

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
Copy link
Collaborator

arokem commented Oct 7, 2023

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.

@arokem arokem merged commit 86d5c98 into tee-ar-ex:master Oct 7, 2023
13 checks passed
@arokem arokem mentioned this pull request Oct 8, 2023
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.

None yet

3 participants