Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Densities, random walks & travelling salesman #206
Densities, random walks & travelling salesman #206
Changes from 4 commits
a26e6ca
0f12b7e
e72f52e
1665906
55b3ba4
74b08f8
362ca9f
fcf699b
fdcda55
3d2d043
2f631ca
83b9081
3fc0bba
82172be
34144b8
e7589f2
432df36
4ff1bf0
cbb1f83
e60da32
2f34957
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a more generic density defined in Eq. 10 in https://onlinelibrary.wiley.com/doi/full/10.1002/mrm.29702 ?
Its a very short update.
Also, having support for elliptical sampling enabled or not can be helpful (we soon plan to disable it :P )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, this density is already based on that equation. What difference do you see ?
And what do you mean by elliptical sampling ? Anisotropic ? Why do you plan to disable it and from where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anisotropic and more importantly elliptical sampling, a way to enable or disable mask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow that dodges all of my questions xD Could you reformulate pls ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support elliptical or non elliptical density
We also need to support anisotropy, but I think that we already do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to fix it but I think it can now handle anisotropy and elliptical densities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to be doing \Psi F*, the paper describes F* \psi right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I based this on the original code from Nicolas:
I checked that the code had an identical behavior, and it is the case except for one major difference: he used hardcoded wavelets banks and I use
pywavelets
instead, and their values seem to differ.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep in mind: https://arxiv.org/pdf/1910.14513, some other interesting results with Anna.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something specific in the paper you would like to highlight ? Should we open an issue to create some extension in the future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @chaithyagr I note in the paper figure that there is a TWIRL implementation sleeping somewhere in our cardboards. Do you know where it is so it can be added to the package ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt TWIRL just TPI in 2D? It should be fairly straightforward from our codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @philouc can comment better than me. I just felt its good to add in docs, Its been long having gone through this work, but from what I remember its extension of the earlier work for anisotropic sampling, which can be of interest too.. But surely not to be pursued in this work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to vectorize this? How slow is it right now for 3D?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paquiteau , maybe numba?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_fast_chauffert_density
here is super fast for any matrix size. It is not the same forcreate_chauffert_density
abovepywavelets
allows the vectorization, but forcreate_chauffert_density
we are speaking about an NxN matrix with N the total number of voxels. We explicit the linear operator to find for each row the max L2 norm. There are probably smart ways to save some computations though.