InsightSoftwareConsortium / ITKIsotropicWavelets

External Module for ITK, implementing Isotropic Wavelets and Riesz Filter for multiscale phase analysis.
Apache License 2.0
13 stars 11 forks source link

ENH: Where https works, replace http using http_to_https.bash #152

Closed tbirdso closed 2 years ago

tbirdso commented 2 years ago

Automated reformatting with https://github.com/InsightSoftwareConsortium/ITK/blob/master/Utilities/Maintenance/ApplyScriptToRemotes.sh

tbirdso commented 2 years ago

Seeing build failures specific to this PR on update:

[13/104] Generating ../../../../../../ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.xml
FAILED: ../../../ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.xml /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.xml 
cd /work/_skbuild/linux-x86_64-3.8/cmake-build/Wrapping/Modules/IsotropicWavelets && /work/_skbuild/linux-x86_64-3.8/cmake-build/Wrapping/Generators/CastXML/castxml/bin/castxml -o /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.xml --castxml-gccxml --castxml-start _wrapping_ --castxml-cc-gnu "(" /opt/rh/devtoolset-10/root/usr/bin/g++ -std=c++14 ")" -w -c @/work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/IsotropicWavelets.castxml.inc /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.cxx
In file included from /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.cxx:21:
In file included from /work/include/itkWaveletFrequencyForward.h:27:
In file included from /work/include/itkFrequencyShrinkImageFilter.h:23:
/work/ITK-source/ITK/Modules/Core/Common/include/itkEnableIf.h:167:4: error: Use C++ 11 std::enable_if directly
#  error Use C++ 11 std::enable_if directly
   ^
In file included from /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.cxx:21:
In file included from /work/include/itkWaveletFrequencyForward.h:28:
In file included from /work/include/itkFrequencyShrinkViaInverseFFTImageFilter.h:21:
/work/include/itkShrinkDecimateImageFilter.h:123:12: error: no template named 'EnableIfC'; did you mean 'Eigen::internal::EnableIf'?
  typename EnableIfC<std::numeric_limits<TOutputType>::is_integer, TOutputType>::Type
           ^~~~~~~~~
           Eigen::internal::EnableIf
/work/ITK-source/ITK/Modules/ThirdParty/Eigen3/src/itkeigen/../itkeigen/Eigen/src/Core/util/ForwardDeclarations.h:151:23: note: 'Eigen::internal::EnableIf' declared here
template<bool> struct EnableIf;
                      ^
In file included from /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.cxx:21:
In file included from /work/include/itkWaveletFrequencyForward.h:28:
In file included from /work/include/itkFrequencyShrinkViaInverseFFTImageFilter.h:21:
/work/include/itkShrinkDecimateImageFilter.h:123:12: error: too many template arguments for class template 'EnableIf'
  typename EnableIfC<std::numeric_limits<TOutputType>::is_integer, TOutputType>::Type
           ^                                                       ~~~~~~~~~~~~
/work/ITK-source/ITK/Modules/ThirdParty/Eigen3/src/itkeigen/../itkeigen/Eigen/src/Core/util/ForwardDeclarations.h:151:23: note: template is declared here
template<bool> struct EnableIf;
~~~~~~~~~~~~~~        ^
In file included from /work/ITK-cp38-cp38-manylinux2014_x64/Wrapping/itkWaveletFrequencyForward.cxx:21:
In file included from /work/include/itkWaveletFrequencyForward.h:28:
In file included from /work/include/itkFrequencyShrinkViaInverseFFTImageFilter.h:21:
/work/include/itkShrinkDecimateImageFilter.h:132:12: error: no template named 'DisableIfC'
  typename DisableIfC<std::numeric_limits<TOutputType>::is_integer, TOutputType>::Type
           ^
4 errors generated.
phcerdan commented 2 years ago

@dzenanz This must be related to some changes in ITK. Could you provide more context? itkEnableIf.h:167:4: error: Use C++ 11 std::enable_if directly

dzenanz commented 2 years ago

I think that https://github.com/InsightSoftwareConsortium/ITK/pull/2713 made the relevant changes. InsightSoftwareConsortium/ITK@98ee48a (#2713) shows what to do.

dzenanz commented 2 years ago

I think the errors come from updating the CI version past the commit which deprecated itk::mpl::EnableIf. The fixes for this can, and should be merged before this PR.

phcerdan commented 2 years ago

Review #153, merge if ok, and rebase on top this branch to check if errors are gone

dzenanz commented 2 years ago

The force-push a plain rebase.

phcerdan commented 2 years ago

This error is fixable, PR incoming

phcerdan commented 2 years ago

154 should fix the ctest failure

phcerdan commented 2 years ago

Why is this PR touching the images? @tbirdso image

dzenanz commented 2 years ago

Those are errors. This was done by a script on ~50 remote modules. A few unwanted file modifications sneaked in.

phcerdan commented 2 years ago

A few unwanted file modifications sneaked in

No doubt. @dzenan could you take over?

dzenanz commented 2 years ago

I am currently travelling. I could take over if no one gets to it before me. But this PR by itself should be easy. CI failures are icky.

phcerdan commented 2 years ago

Is it possible that the original test images have been deleted in the data server? That's why other unrelated PR's to this are also seg-faulting?

dzenanz commented 2 years ago

Lack/absence of data would produce a read exception, not a segfault.

phcerdan commented 2 years ago

It's not lack/absence of file, it's corruption of that file.

tbirdso commented 2 years ago

@dzenanz @phcerdan Thanks for your work on this, and great catch identifying test files that were unintentionally updated. I've removed those changes from this PR.

phcerdan commented 2 years ago

That solved seg-faulting. Thanks @tbirdso. I will have a look at python errors. You are more up-to-date in recent ITK changes. Do you know if something has changed in itkPoint, itkVector, itkFixedArray, constructors or wrapping?

tbirdso commented 2 years ago

I see the errors you are describing, unfortunately I'm not sure what would have changed recently. Perhaps @dzenanz will have some intuition?

/work/include/itkMonogenicSignalFrequencyImageFilter.hxx:57:84: error: cannot convert ‘itk::FrequencyFFTLayoutImageRegionConstIteratorWithIndex<itk::Image<std::complex, 3> >::FrequencyType’ {aka ‘itk::Vector<double, 3>’} to ‘const itk::Point<double, 3>&’ 57 evaluatedArray = this->m_Evaluator->EvaluateAllComponents(inFreqIt.GetFrequency()); ~~~~~^~
dzenanz commented 2 years ago

I think this was due to increasing strictness in types. I think @N-Dekker did it in main repo.

N-Dekker commented 2 years ago

I think this was due to increasing strictness in types. I think @N-Dekker did it in main repo.

Indeed! 🤔 [still looking at the code...] Do I understand correctly that GetFrequency() returns an itk::Vector, while EvaluateAllComponents expects an itk::Point as argument?

A itk::Vector may be "converted" to a point by adding the vector to a zero-filled point. So a quickfix could be to add a zero-filled point to the return value of GetFrequency(). Does that help?

phcerdan commented 2 years ago

156 add extra wrappings for FrequencyFunction and derived classes to fix wrappings errors. Please rebase this after #156 is merged.

phcerdan commented 2 years ago

By the way, thanks @N-Dekker! I opted to wrap more types instead of converting or casting them.

phcerdan commented 2 years ago

I got segfaults in tests related to data corruption generated in this PR in #156, even though they are solved in the last iteration of this PR. Is the ExternalData cached or something in the servers? I fail to understand :(

tbirdso commented 2 years ago

I see checks are now passing in the master branch, thank you for your work here @phcerdan !