Benchmark tweaks #62
Benchmark tweaks #62
Conversation
3dca15b
to
5f040ed
Compare
e59d773
to
c9c85ea
Compare
204a2c8
to
bbf88a8
Compare
f86df59
to
4fb8ea7
Compare
4fb8ea7
to
8a7ed16
Compare
|
Alright, a lot happened but now:
Moving forward
I think these could be addressed in later PRs. |
| backend: [finufft, pynfft, pynufft, bart, sigpy] | ||
| exclude: | ||
| - backend: bart | ||
| - backend: pynfft |
There was a problem hiding this comment.
Maybe add an issue that it should be fixed and also point the issue here.
| @@ -0,0 +1,52 @@ | |||
| """TEST CONFIGURATION. | |||
There was a problem hiding this comment.
Why is this file in examples?
There was a problem hiding this comment.
This allows the examples to be run as tests 😉, the examples are the best solution to test the trajectories generation and display IMO.
see https://stackoverflow.com/questions/56807698/how-to-run-script-as-pytest-test for more infos.
| # Convert to float array and take only the real part | ||
| traj = np.ascontiguousarray(traj_raw.T.view("(2,)float32")[..., 0]) | ||
| if np.all(traj[..., -1] == 0): | ||
| warnings.warn("2D Trajectory Detected") |
There was a problem hiding this comment.
Why do we need a warning? is it when we expect a 3D trajectory?
There was a problem hiding this comment.
Because this might also be a valid 3D trajectory (Sampling the central slice of the space of a 3D object, but here only a 2D NUFFT will be done by BART.
| @@ -342,25 +335,29 @@ def op(self, data, ksp_d=None): | |||
| else: | |||
| op_func = self._op_calibless_host | |||
| ret = op_func(data, ksp_d) | |||
| if not self.persist_plan: | |||
There was a problem hiding this comment.
I dont know why persist plan option is removed?
There was a problem hiding this comment.
This was too much hassle to maintain internally, and had little to no usage in practice. If ones want to have "volatile" plans he can simply delete/create new nufft operators.
| return self._safe_squeeze(ret) | ||
|
|
||
| def _data_consistency_sense(self, image_data, obs_data): |
There was a problem hiding this comment.
I prefer the old name somehow. grad_sense while seems cool, could be confusing in future when we would want to make the flow compatible with autodiff.
| @@ -584,7 +575,7 @@ def __adj_op(self, coeffs_d, image_d): | |||
| ret = self.raw_op.type1(coeffs_d, image_d) | |||
| return ret | |||
|
|
|||
| def data_consistency(self, image_data, obs_data): | |||
| def get_grad(self, image_data, obs_data): | |||
| "kspace_traj, shape", | ||
| cases=[CasesTrajectories.case_radial3D], | ||
| ) | ||
| @parametrize("backend", ["cufinufft", "tensorflow"]) |
There was a problem hiding this comment.
Why was this removed? I understand that it must be tested upstream, but this wont take much time and can be very useful in future to see if gpuNUFFT breaks.
There was a problem hiding this comment.
Pipe Method is not converging. In lack of a better solution I removed it.
| return np.sqrt(2 ** len(self.shape)) | ||
|
|
||
|
|
||
| def _readcfl(cfl_file, hdr_file=None): |
There was a problem hiding this comment.
It might as well be helpful to have this exposed (even the writer). We have collaborators who use CFL files for storing k-space data
There was a problem hiding this comment.
Sure, this could go in a proper io module (I guess we will tackle that in another PR) see #65
|
Regarding the renaming of |
This PR adds all the tweak done while working on the benchmark. A lot of happened, and despite my best effort is hard do split it up in several chunks