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
Feature/line integrator #1
base: master
Are you sure you want to change the base?
Feature/line integrator #1
Changes from 1 commit
7067dd3
4d39d8a
06c36a7
c4c4a58
f5bc98a
463b054
bc8ac0b
d20fb73
5f29bbc
2fd95d1
6a02598
8fab8ac
0c8d4cc
7759876
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.
This should be resolved before the merge. See the todo note.
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.
Ok, a simple solution would be to add an optional parameter
integral_method='trapz'
in a similar fashion as you do in pleque and add a warning about this into the docstring.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 suppose that
method='sum'
would be something likeintergand.sum(integration_dim) * dl
?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.
Nope.
integrate
here only do "summation" of elements of theintegrand
, but with Trapez rule (see the example below).dl
element is already included inj
.biot_savart_integrators/biot_savart_integrators/generic/curve.py
Line 12 in 32d2947
If you assign some coordinates (for example
l = cumsum(dl)
) the integration will brake.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.
A possible solution would be something like this (untested; adjusted your function):
or
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.
Did you mean
integrand.coords[integration_dim] = np.cumsum(dl)
?I suppose the simplest solution would be just to use
.sum()
instead of.integrate()
. Perhaps the possible inaccuracy ofsum
is not worth all these workarounds?