refactor potential field (boundary) construction - #33
Open
ackl wants to merge 2 commits into
Open
Conversation
* centralize Greens function evaluation in compute_scalar_potential * route FFT and direct boundary construction through get_potential_boundary * consolidate full and top potential boundary datasets * simplify loader responsibilities BREAKING CHANGE: remove get_potential, get_potential_top, load_fft_potential_field_boundary, Use get_potential_field or get_potential_boundary instead.
* 'method' key in yaml config was not being wired thru to potential field construction code, despite docs saying that setting 'direct' in the config should be able to use the green's function fallback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
for a current use case i'm importing the nf2 methods for calculating potential field values and i saw some areas that could be cleaned up a bit:
This PR consolidates the FFT and direct green's function potential field implementations behind a common API and simplifies the responsibilities of it's callers. i used the Low & Lou analytical field to test before and after the changes on this branch and the values are equal
centralises greens function evaluation in
compute_scalar_potentialroute both fft and direct boundary construction through
get_potential_boundaryonly_topargument so the responsibility of extracting just the top face isnt in theloader.pymoduleconsolidate
PotentialBoundaryDatasetandPotentialTopBoundaryDatasetonly_toparg wasn't being passed but thedocs imply it should be)get_fft_potential_fieldhas been trimmed down a bit so we don't have to create the intermediate*_extnumpy arraysHxB[0, 0] = -1jHyB[0, 0] = -1jassignments are for, so i've left it in for nowthe following methods
get_potential,get_potential_top,load_fft_potential_field_boundaryhave been removed and are replaced withget_potential_field,get_potential_boundary(..., only_top=True),load_potential_field_boundary(..., only_top=True).