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

use naturalneighbour interpolation instead of linear barycentric #306

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

jordandekraker
Copy link
Collaborator

@jordandekraker jordandekraker commented Jun 26, 2024

This PR adds only the best changes from #300 and #305.

Notably, from #300 no changes are kept except those that make surface config easier to use. Also interpolation is performed in 256x128x16 unfolded space instead of 0-1x0-1x0-1 unfolded space. Other changes are no longer needed since they are handled in #305.

From #305, here are the major changes:

Naturalneighbor interpolation is in create_warps.py. This lovely interpolation function is fast and initial tests show it working extremely well, solving all issues in #300 without the need for any postprocessing of surfaces. I tried many things to clean these surfaces, including different interpolation functions, but I believe this fix now finally solves the root cause.

This is a breaking change since outputs will now be slightly different (and better quality). I see no downsides or anticipated conflicts.

This is also now in-line with the original Matlab code that was used for interpolation (hippo_autotop)

Still doing a wet-run test.

@jordandekraker
Copy link
Collaborator Author

@akhanf please merge khanlab/hippunfold_deps#12 before reviewing this PR

@akhanf
Copy link
Member

akhanf commented Jun 26, 2024

Great! I'll try to have a look soon, and also build the design container (lock file needs updating first I think).

interp_io = griddata(
points, values=native_coords_phys[:, 2], xi=unfold_xi, method=interp_method
# perform the interpolation

Copy link
Member

Choose a reason for hiding this comment

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

actually looks like noise is added here - so just need to move the comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, unfolded_gx unfolded_gy unfolded_gz were no longer used, so I removed them. This also means that epsilon is no longer used, and i believe its no longer needed

@jordandekraker
Copy link
Collaborator Author

OK i cleaned up the reviews. One more question: does https://github.com/khanlab/hippunfold_deps build the hippunfold_deps container automatically or do i need to trigger it?

@akhanf akhanf changed the title fresh branch for clean changes use naturalneighbour interpolation instead of linear barycentric Jun 26, 2024
@akhanf
Copy link
Member

akhanf commented Jun 26, 2024

I've made a hippunfold_deps release (v0.5.1) which triggers the docker hub build. Will need to update the container version here to point to that too.

@jordandekraker jordandekraker added the breaking New feature that breaks comptability with previous versions label Jul 2, 2024
@jordandekraker jordandekraker merged commit c32bac2 into master Jul 8, 2024
5 checks passed
@jordandekraker jordandekraker deleted the cleanwarpgen branch July 8, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking New feature that breaks comptability with previous versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants