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

Implementing off-grid sensors #170

Closed
tomelse opened this issue Jun 9, 2023 · 4 comments
Closed

Implementing off-grid sensors #170

tomelse opened this issue Jun 9, 2023 · 4 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@tomelse
Copy link
Contributor

tomelse commented Jun 9, 2023

Hi! I thought it would be really nice to introduce off-grid sensors into j-Wave, as they have done in k-Wave (http://www.k-wave.org/forum/topic/alpha-version-of-kwavearray-off-grid-sources).

I've implemented a basic version of this myself using the band-limited interpolant, which you can see in this gist: https://gist.github.com/tomelse/f9ba7508b75f44f34ebdbf25d5f5b0a3.

I'm happy to put a pull request in myself, I just wanted to get some feedback on how best to integrate it into the j-Wave code.

bli_comparison

The results look really nice, removing all the staircasing that you can see in the on-grid version, which you can see in an example simulation above (x axis = time, y axis= detector). It's a little bit slower than the original j-Wave implementation but not prohibitively so (maybe that's to be expected?).

@tomelse tomelse added enhancement New feature or request question Further information is requested labels Jun 9, 2023
@astanziola
Copy link
Member

Hello Thomas. That looks great, thanks a lot! Please do feel absolutely free to open a PR, it would be amazing to have off-grid sensors.

I think this can easily go into the geometry.py file almost as is, although there may be a good argument to start having two separate sources and sensors modules.

I was looking at the comment given by @jgroehl below your gist and I think I agree with him. I can imagine a situation when someone want so do something like

def my_loss_func(sensors_positions: Tuple[np.typing.ArrayLike]:
    sensors = BLISensors(sensors_positions)
    ...
    
sensor_positions_grad = jax.grad(my_loss_func)

In that way the position of the sensors can also be tuned by gradient descent! In that case, the flattening and unflattening methods also need to be changed in the pytree definition.

It would also be good to add some testing for this, for example by reproducing the followings:

But the way the testing is handled in this repo currently is a bit clunky (any suggestion is more than welcome!), so I'm happy to add them afterwards

Thank you for taking time to do this and share it!h

@astanziola
Copy link
Member

Hello @tomelse ! Just gauging if you think you would like to make a PR on this? Otherwise I'm happy to take this over (of course acknowledging your contribution!)

@tomelse
Copy link
Contributor Author

tomelse commented Jun 23, 2023

I'll have a look next week, had such a hectic week!

@tomelse
Copy link
Contributor Author

tomelse commented Jun 27, 2023

@astanziola I've implemented this in a new pull request (#192). There's example code, test code and an updated implementation.

@tomelse tomelse closed this as completed Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants