c3d icon indicating copy to clipboard operation
c3d copied to clipboard

Provide conda-forge package

Open ghisvail opened this issue 3 years ago • 16 comments

What's the statuts of (and work remaining for) porting Convert3D to ITK version 5?

I can see some initial work on the itk5 branch and a pending PR #8.

I'd be happy to help.

ghisvail avatar May 31 '21 09:05 ghisvail

Branch itk5 is fully functional. It works with 5.1.2. Please let me know if you encounter any issues.

On Mon, May 31, 2021 at 5:16 AM Ghislain Antony Vaillant < @.***> wrote:

What's the statuts of (and work remaining for) porting Convert3D to ITK version 5?

I can see some initial work on the itk5 branch and a pending PR #8 https://github.com/pyushkevich/c3d/pull/8.

I'd be happy to help.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEW2RISFC6XLUIRWESYLTQNHXRANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar May 31 '21 11:05 pyushkevich

I am trying to build a conda-forge package for convert3d and only ITK 5 is available hence my request.

So far, I am stuck with the MorphologicalContourInterpolation dependency which is not available right now. It is still a ITK Remote module and they are not built by default on conda-forge.

Have you got any plans to merge it to the main branch at some point and tag a new release?

ghisvail avatar Jun 02 '21 09:06 ghisvail

Hi Ghislain

Having c3d in conda would be great, and if you could also do the same four our greedy registration tool (https://github.com/pyushkevich/greedy), that would be great too.

I added a CMake option CONVERT3D_USE_ITK_REMOTE_MODULES (which is ON by default). If you set it to OFF, it will no longer look for ITK remote modules. I also merged itk5 branch into the main one.

Thanks, Paul

On Wed, Jun 2, 2021 at 5:32 AM Ghislain Antony Vaillant < @.***> wrote:

I am trying to build a conda-forge package for convert3d and only ITK 5 is available hence my request.

So far, I am stuck with the MorphologicalContourInterpolation dependency which is not available right now. It is still a ITK Remote module and they are not built by default on conda-forge.

Have you got any plans to merge it to the main branch at some point and tag a new release?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-852869811, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEWZPCOKJYSZD7CK5XATTQX3ERANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar Jun 02 '21 13:06 pyushkevich

I added a CMake option CONVERT3D_USE_ITK_REMOTE_MODULES

Brilliant. Thank you so much.

Progress on the conda-forge package is here btw:

https://github.com/conda-forge/staged-recipes/pull/15090

ghisvail avatar Jun 02 '21 13:06 ghisvail

@pyushkevich fyi, you've got a few deprecated usage of VNL:

2021-06-02T13:50:33.0095954Z ../utilities/AffineTransformTool.cxx: In function 'void quart_print(MatrixType&)':
2021-06-02T13:50:33.0103364Z ../utilities/AffineTransformTool.cxx:224:22: warning: 'vnl_matrix_fixed<T, num_rows, num_cols>::operator const vnl_matrix_ref<T>() const [with T = double; unsigned int num_rows = 3; unsigned int num_cols = 3]' is deprecated: Implicit cast conversion is dangerous.\nUSE: .as_matrix() or .as_ref() member function for clarity. [-Wdeprecated-declarations]
2021-06-02T13:50:33.0109852Z   224 |   vnl_qr<double> qr(A);
2021-06-02T13:50:33.0115455Z       |                      ^
2021-06-02T13:50:33.0122988Z In file included from $PREFIX/include/ITK-5.2/vnl/vnl_matrix_fixed.hxx:9,
2021-06-02T13:50:33.0138077Z                  from $PREFIX/include/ITK-5.2/itkMatrix.h:25,
2021-06-02T13:50:33.0139307Z                  from $PREFIX/include/ITK-5.2/itkImageBase.h:34,
2021-06-02T13:50:33.0140319Z                  from $PREFIX/include/ITK-5.2/itkNeighborhoodAccessorFunctor.h:22,
2021-06-02T13:50:33.0141315Z                  from $PREFIX/include/ITK-5.2/itkImage.h:28,
2021-06-02T13:50:33.0142074Z                  from ../itkextras/itkOrientedRASImage.h:4,
2021-06-02T13:50:33.0142942Z                  from ../utilities/AffineTransformTool.cxx:26:
2021-06-02T13:50:33.0143874Z $PREFIX/include/ITK-5.2/vnl/vnl_matrix_fixed.h:687:3: note: declared here
2021-06-02T13:50:33.0144879Z   687 |   operator const vnl_matrix_ref<T>() const { return  this->as_ref(); }
2021-06-02T13:50:33.0145535Z       |   ^~~~~~~~
2021-06-02T13:50:33.0146426Z ../utilities/AffineTransformTool.cxx: In function 'void irtk_write(MatrixStack&, const char*)':
2021-06-02T13:50:33.0148302Z ../utilities/AffineTransformTool.cxx:353:22: warning: 'vnl_matrix_fixed<T, num_rows, num_cols>::operator const vnl_matrix_ref<T>() const [with T = double; unsigned int num_rows = 3; unsigned int num_cols = 3]' is deprecated: Implicit cast conversion is dangerous.\nUSE: .as_matrix() or .as_ref() member function for clarity. [-Wdeprecated-declarations]
2021-06-02T13:50:33.0149572Z   353 |   vnl_qr<double> qr(A);
2021-06-02T13:50:33.0150115Z       |                      ^
2021-06-02T13:50:33.0150942Z In file included from $PREFIX/include/ITK-5.2/vnl/vnl_matrix_fixed.hxx:9,
2021-06-02T13:50:33.0151939Z                  from $PREFIX/include/ITK-5.2/itkMatrix.h:25,
2021-06-02T13:50:33.0152883Z                  from $PREFIX/include/ITK-5.2/itkImageBase.h:34,
2021-06-02T13:50:33.0154181Z                  from $PREFIX/include/ITK-5.2/itkNeighborhoodAccessorFunctor.h:22,
2021-06-02T13:50:33.0155183Z                  from $PREFIX/include/ITK-5.2/itkImage.h:28,
2021-06-02T13:50:33.0156182Z                  from ../itkextras/itkOrientedRASImage.h:4,
2021-06-02T13:50:33.0156924Z                  from ../utilities/AffineTransformTool.cxx:26:
2021-06-02T13:50:33.0158004Z $PREFIX/include/ITK-5.2/vnl/vnl_matrix_fixed.h:687:3: note: declared here
2021-06-02T13:50:33.0159361Z   687 |   operator const vnl_matrix_ref<T>() const { return  this->as_ref(); }
2021-06-02T13:50:33.0160026Z       |   ^~~~~~~~

ghisvail avatar Jun 02 '21 13:06 ghisvail

There is also an occurrence of: FIND_LIBRARY(FFTW_LIB fftw3f) but it looks like it's not used anywhere directly and the conda-forge tooling complains of overlinkage.

Aren't FFT features provided by ITK in your case?

ghisvail avatar Jun 02 '21 14:06 ghisvail

Apart from that, the binaries build fine on the conda-forge infrastructure so we are quite close modulo the overlinking issue.

ghisvail avatar Jun 02 '21 14:06 ghisvail

Hi, just pushed a commit removing the FFTW line from CMakeLists

Thanks!

On Wed, Jun 2, 2021 at 10:06 AM Ghislain Antony Vaillant < @.***> wrote:

Apart from that, the binaries build fine on the conda-forge infrastructure so we are quite close modulo the overlinking issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-853057948, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEW2Y72QHJ3TEKRFTA7TTQY3GLANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar Jun 02 '21 20:06 pyushkevich

Actually, you've got and explicit dependency on FFTW here.

ghisvail avatar Jun 02 '21 21:06 ghisvail

Thanks, I missed that. The file is not used, and I just pushed a commit deleting it.

On Wed, Jun 2, 2021 at 5:53 PM Ghislain Antony Vaillant < @.***> wrote:

Actually, you've got and explicit dependency on FFTW here https://github.com/pyushkevich/c3d/blob/master/adapters/SimpleElasticRegistration.h .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-853408390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEW2ZHEDWJ2YKL633WCDTQ2R5ZANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar Jun 03 '21 13:06 pyushkevich

Thanks, I am rebasing the package onto your latest commit.

Do you want to be added as co-maintainer of the conda-forge repo?

ghisvail avatar Jun 03 '21 14:06 ghisvail

Thanks, sure, I don't really know how to package stuff for conda though.

On Thu, Jun 3, 2021 at 10:54 AM Ghislain Antony Vaillant < @.***> wrote:

Thanks, I am rebasing the package onto your latest commit.

Do you want to be added as co-maintainer of the conda-forge repo?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-853932247, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEW3PDNYIRL5FQWM3OETTQ6JQVANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar Jun 03 '21 17:06 pyushkevich

That's the nice thing, once it's done the only maintenance effort is just merging software upgrade PRs proposed by the conda-forge CI.

Le jeu. 3 juin 2021 à 19:21, Paul Yushkevich @.***> a écrit :

Thanks, sure, I don't really know how to package stuff for conda though.

On Thu, Jun 3, 2021 at 10:54 AM Ghislain Antony Vaillant < @.***> wrote:

Thanks, I am rebasing the package onto your latest commit.

Do you want to be added as co-maintainer of the conda-forge repo?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-853932247, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAJPEW3PDNYIRL5FQWM3OETTQ6JQVANCNFSM452QFQRQ

.

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-854044491, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO7U33I4GPT4DW2EZY5JP3TQ62ZXANCNFSM452QFQRQ .

ghisvail avatar Jun 03 '21 17:06 ghisvail

The conda-forge package is ready for review. @pyushkevich would you mind tagging the current version of the code with a new one (v1.3.0?) so I can reference a proper release tarball in the conda-forge recipe?

Cheers.

ghisvail avatar Jun 07 '21 16:06 ghisvail

Thanks, Ghislain! I just tagged v1.3.0 and pushed the tag.

On Mon, Jun 7, 2021 at 12:25 PM Ghislain Antony Vaillant < @.***> wrote:

The conda-forge package is ready for review. @pyushkevich https://github.com/pyushkevich would you mind tagging the current version of the code with a new one (v1.3.0?) so I can reference a proper release tarball in the conda-forge recipe?

Cheers.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pyushkevich/c3d/issues/9#issuecomment-856081661, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJPEW2MR7XAXXTX4WFAOJTTRTXHZANCNFSM452QFQRQ .

-- Paul A. Yushkevich, Ph.D. Professor of Radiology Penn Image Computing and Science Laboratory University of Pennsylvania Perelman School of Medicine

pyushkevich avatar Jun 07 '21 19:06 pyushkevich

I check the conda-forge recipe, and found:

export CFLAGS="${CFLAGS} -I ${PREFIX}/include/eigen3"
export CXXFLAGS="${CXXFLAGS} -I ${PREFIX}/include/eigen3"

so, why not add -I ${PREFIX}/include/eigen3 to https://github.com/pyushkevich/c3d/blob/master/CMakeLists.txt? add this line:

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -I/usr/include/eigen3")

to https://github.com/pyushkevich/c3d/blob/master/CMakeLists.txt line 18.

hubutui avatar Mar 07 '22 13:03 hubutui

Forgot to close this issue. A conda package has been available for a while now.

ghisvail avatar Jul 20 '23 14:07 ghisvail