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

adjoint filter cleanup and additions #1093

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Conversation

tylerflex
Copy link
Collaborator

@tylerflex tylerflex commented Aug 24, 2023

  • Adds a CircularFilter, which is just a uniform "cylinder" kernel instead of a conic kernel.
  • Rename feature_size to radius in the circular filters.
  • Remove extra factor of sqrt(3) in the radius calculation, which is inconsistent with literature.

Note: initial testing of the circular filter is showing that it's terrible compared to conic filter, so not mentioning it in the changelog for now.

@tylerflex tylerflex marked this pull request as draft August 24, 2023 15:25
@tylerflex tylerflex force-pushed the tyler/adjoint/utils3 branch 4 times, most recently from 829a939 to d7ace19 Compare August 28, 2023 21:30
@tylerflex tylerflex marked this pull request as ready for review August 28, 2023 21:31
@tylerflex tylerflex added the 2.4 label Aug 28, 2023
@tylerflex
Copy link
Collaborator Author

Made edits based on Emerson's comments

1 - The descriptions for the Conic and Circular filters are the same.
2 - The title of vmax on line 134 should be "Max value".
3 - As the filter radius is general, maybe describe it using only "Radius of the circular filter to convolve with supplied spatial data." in line 27.

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Looks good, just some epxlanation about the difference between conical and circular filter somehwere?

@tylerflex
Copy link
Collaborator Author

Thanks, I added a bit more explanation and recommendation that the user use a conical mask (note each of them have the formula of the kernel already).

@tylerflex tylerflex merged commit 231f15c into pre/2.4 Aug 30, 2023
16 checks passed
@tylerflex tylerflex deleted the tyler/adjoint/utils3 branch August 30, 2023 16:32
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.

2 participants