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

Python package #605

Merged
merged 7 commits into from
Jul 15, 2022
Merged

Python package #605

merged 7 commits into from
Jul 15, 2022

Conversation

casperdcl
Copy link
Contributor

casperdcl added a commit to NiftyPET/NIMPA that referenced this pull request May 9, 2022
@casperdcl casperdcl mentioned this pull request May 9, 2022
4 tasks
@neurolabusc
Copy link
Collaborator

Looks good to me. @ningfei do you have any comments?

@ningfei
Copy link
Collaborator

ningfei commented May 10, 2022

Looks good to me. Only one thing: is cmake_args=[f"-DCM2NIIX_BUILD_VERSION={build_ver}", "-DBUILD_ALL_DEP=ON"]) needed for the python package? Since we don't have the two options in the SuperBuild.cmake. Besides, there's a typo here: -DCM2NIIX_BUILD_VERSION={build_ver} should be -DDCM2NIIX_BUILD_VERSION={build_ver}.

@neurolabusc
Copy link
Collaborator

@casperdcl can you respond to @ningfei 's comments. I am eager to push a belated spring release.

@casperdcl
Copy link
Contributor Author

Sorry for the delay @neurolabusc @ningfei, just pushed some changes.

  • BUILD_ALL_DEP: good catch, this was leftover from a different project unrelated to dcm2niix
  • DCM2NIIX_BUILD_VERSION: indeed, missing a D prefix. It's usually nice to define this sort of thing for external C-libraries to potentially use, but I agree probably not worth it seeing as dcm2niix doesn't currently us it.

@casperdcl
Copy link
Contributor Author

casperdcl commented Jul 15, 2022

Hey @neurolabusc is this good to go now?

@neurolabusc
Copy link
Collaborator

@ningfei can you merge this if you are happy with this. I am overdue for a new stable release, but I am waiting for a bit more feedback on XA30 conversion.

@ningfei
Copy link
Collaborator

ningfei commented Jul 15, 2022

sure!

@ningfei ningfei merged commit 7e86893 into rordenlab:development Jul 15, 2022
@casperdcl
Copy link
Contributor Author

Hey @ningfei @neurolabusc - despite being merged, the changes from this PR don't appear to be anywhere in development and/or master

@neurolabusc
Copy link
Collaborator

@ningfei this seems to be in your area of expertise. Tell me if you are too busy to address this.

@ningfei
Copy link
Collaborator

ningfei commented Nov 24, 2022

That's weird. I'm sure I saw these files in development branch back then. Somehow they are missing now. Will redo the merge. @neurolabusc shall I also merge these commits into master?

@ningfei ningfei mentioned this pull request Nov 24, 2022
@neurolabusc
Copy link
Collaborator

@ningfei I would be happy to update the master to create a new stable release. However, both issues 629 and 647 seem to fall in your expertise. Would you please respond to each of those (even if it is just to acknowledge them) prior to updating the master branch.

@ningfei
Copy link
Collaborator

ningfei commented Nov 26, 2022

@neurolabusc will have a look soon. For the CI, probably I will still use AppVeyor for now. Since Github Actions doesn't support old images like Ubuntu 16.04 or macOS Cataline.

casperdcl added a commit to casperdcl/dcm2niix that referenced this pull request Jul 3, 2024
@casperdcl casperdcl mentioned this pull request Jul 3, 2024
casperdcl added a commit to casperdcl/dcm2niix that referenced this pull request Jul 3, 2024
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