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

Adding 3D wavelets from Sparse3D #52

Merged
merged 15 commits into from
Jul 25, 2018
Merged

Conversation

bensarthou
Copy link
Contributor

Adding 3D transforms from the Sparse2D package (https://github.com/CosmoStat/Sparse2D/)

  • Main modifications: Adding cpp files in pysparse to have the bindings
  • Modify wavelet API in base/transforms and extensions/transforms to use 3D
  • Add the transforms in extensions
  • Add some tests

WARNING: This PR would pass only after the PR #16 of Sparse2D has passed, as it correct a memory bug vital for using the wavelets.

@bensarthou bensarthou changed the title PR sparse3 d PR sparse3D Jul 18, 2018
@bensarthou bensarthou changed the title PR sparse3D Adding 3D wavelets from Sparse3D Jul 18, 2018
@coveralls
Copy link

coveralls commented Jul 20, 2018

Pull Request Test Coverage Report for Build 148

  • 48 of 100 (48.0%) changed or added relevant lines in 7 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.4%) to 56.017%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pysap/plugins/mri/reconstruct/linear.py 6 8 75.0%
pysap/base/transform.py 7 13 53.85%
pysap/extensions/tools.py 3 47 6.38%
Files with Coverage Reduction New Missed Lines %
pysap/numerics/reconstruct.py 1 52.48%
pysap/data.py 5 40.82%
Totals Coverage Status
Change from base Build 141: -0.4%
Covered Lines: 1336
Relevant Lines: 2385

💛 - Coveralls

@AGrigis
Copy link
Contributor

AGrigis commented Jul 24, 2018

The tests on Travis failed for Python 2.7:

ERROR: Test all the registered transformations.

Traceback (most recent call last):
File "/home/travis/build/CEA-COSMIC/pysap/pysap/test/test_binding.py", line 71, in test_wavelet_transformations
rtol=1e-5)))
File "/home/travis/build/CEA-COSMIC/pysap/install/numpy/core/numeric.py", line 2335, in isclose
return within_tol(x, y, atol, rtol)
File "/home/travis/build/CEA-COSMIC/pysap/install/numpy/core/numeric.py", line 2321, in within_tol
return less_equal(abs(x-y), atol + rtol * abs(y))
ValueError: operands could not be broadcast together with shapes (128,128,128) (8388608,)

@AGrigis
Copy link
Contributor

AGrigis commented Jul 24, 2018

I also notice this issue when installing nose with Python 2.7:

mkl-random 1.0.1 requires cython, which is not installed.

@@ -16,5 +16,8 @@
from .tools import mr_filter
from .tools import mr_deconv
from .tools import mr_recons
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to rename the previous tools to distinguish between 1d, 2d, and 3d transforms?
mr1d_*
mr2d_*
mr3d_*
If yes, just use the import as synthax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mr*_ binds 1D and 2D, it is not a necessity

if type_of_multiresolution_transform == 1:
if type_of_filters == 10:
raise ValueError('Wrong type of filters with orthogonal transform')
if type_of_lifting_transform != 3 and\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid using \ and prefer ().


# A trous wavelet transform
if type_of_multiresolution_transform == 3:
if type_of_lifting_transform != 3 and\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem.

else:
self.trf = None
if self.data_dim == 2:
self.trf = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.trf is already set to None.
To remove.

@@ -110,8 +111,18 @@ def __init__(self, nb_scale, verbose=0, **kwargs):
self.__isap_transform_id__)
kwargs["number_of_scales"] = self.nb_scale
self.trf = pysparse.MRTransform(**self.kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keeping this line?

// Get the data: force cast to float
// TODO: use buffer
fltarray im(arr.shape(0), arr.shape(1), arr.shape(2));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

fltarray im(arr.shape(0), arr.shape(1), arr.shape(2));

NumPyArrayData<double> arr_data(arr);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

return im;
}

// convert functions for 3d arrays
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment: not necessary here.

return im;
}

// convert functions for 3d arrays
bn::ndarray image2array_3d(const fltarray& im){
// TODO: use buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the speed was important for you.
Did you try to speed up the binding using the buffer only?
It may really improve the optimization algorithms speed.

}

}


// Module property
bp::scope().attr("__version__") = "0.0.1";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase the binding package version.

@@ -0,0 +1,311 @@
/*##########################################################################
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

##########################################################################

pySAP - Copyright (C) CEA, 2017 - 2018

Distributed under the terms of the CeCILL-B license, as published by

the CEA-CNRS-INRIA. Refer to the LICENSE file or to

http://www.cecill.info/licences/Licence_CeCILL-B_V1-en.html

for details.

##########################################################################

#include "NumPyArrayData.h"



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove one empty line.

cout << " Save transform: " << save << endl;
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove one empty line.

@AGrigis AGrigis merged commit b4713c5 into CEA-COSMIC:master Jul 25, 2018
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.

3 participants