search

CEA-COSMIC / pysap-mri

MRI external plugin for Python Sparse data Analysis Package
Other
43 stars 18 forks source link

CircleCI tests are running for too long #117

Closed chaithyagr closed 3 years ago

chaithyagr commented 3 years ago

Currently, many PRs are blocked particularly because the examples are running for too long.

We need to work with this as many times the things just time out and fail

chaithyagr commented 3 years ago

I see that the major culprit for this is the example non_cartesian_reconstruction_stack_3d.py, which was introduced in the days when we didnt have gpuNUFFT and we carried out reconstructions of 3D NUFFT for stacks by 2D plane NUFFT and 1D FFT. While we still feel this NUFFT could be helpful and will hold on to it for a bit longer and test it with tests (Fourier Adjoint test with stackedFourier), I feel we can remove this example as it misguides people into thinking that this is the best case scenario.

What do you think @zaccharieramzi ?

chaithyagr commented 3 years ago

Further, I am adding here that, this code caused issues even in past, so in examples we limited it to 10 iterations, due to which the final result is worse than a simple gridded solution.

zaccharieramzi commented 3 years ago

How much longer is it compared to the rest of the tests?

I see that in circle ci we are not using a matrix to divide up the testing, so maybe doing that first would reduce the time taken.

chaithyagr commented 3 years ago

Well for now it takes more than 10 minutes to just get the lipchitz constant. This is timeout after which circleci closes the testing. Yes surely adding matrix could be helpful here.

chaithyagr commented 3 years ago

Locally it isnt very long, but that is cause we carry out fourier along z (totally NxN 1D FFTs), and we can afford a lot of threads locally, while circleci fails with it..

zaccharieramzi commented 3 years ago

I see. Then indeed let's not run this example. But I suggest also adding in the matrix.

chaithyagr commented 3 years ago

Yes, will add it on to #118