-
Notifications
You must be signed in to change notification settings - Fork 4
Support maximum current calculation with simulated pulse shape library #134
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
==========================================
- Coverage 70.69% 70.48% -0.21%
==========================================
Files 31 31
Lines 2416 2494 +78
==========================================
+ Hits 1708 1758 +50
- Misses 708 736 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is now ready, the missing items are optional |
…st enough (and cant be used in njit)
abb2e56 to
035bbda
Compare
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.
committed by mistake?
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.
oops sorry yes
| r: ArrayLike, | ||
| z: ArrayLike, | ||
| template: ArrayLike, | ||
| pulse_shape_library: tuple[np.array, np.array, np.array], |
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.
is this annotation correct?
| r: ak.Array, | ||
| z: ak.Array, | ||
| template: np.array, | ||
| pulse_shape_library: tuple[np.array, np.array, np.array], |
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.
again annotation here?
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.
also update 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.
yes its correct, since you cant pass the class to a numba func
| start: float, | ||
| dt: float, | ||
| ) -> float: | ||
| """Get the value of the waveform at a certain index. |
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 docstring is out of date, but maybe it can be made very short since it's not even a public function?
|
let's merge this once the final polishing is done? |
Yes I think it is almost ready, it is also now been tested a bit by @SalehGiovanna |
|
brava |
Some steps are needed to have this working
maximum_currentWe should also think about whether we still use the drift time map, whether we interpolate also in
phietc, how to combine with the surface simulation etc.