Allow adjusting convolution settings for accuracy#159
Allow adjusting convolution settings for accuracy#159henrikjacobsenfys merged 13 commits intodevelopfrom
Conversation
henrikjacobsenfys
added
[scope] enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #159 +/- ##
===========================================
+ Coverage 97.86% 97.93% +0.07%
===========================================
Files 42 43 +1
Lines 2621 2715 +94
Branches 453 472 +19
===========================================
+ Hits 2565 2659 +94
Misses 33 33
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Ready for merge with minor suggestions for refactoring.
| if not self.convolution_settings.convolution_plan_is_valid: | ||
| self._energy_grid = self._create_energy_grid() |
There was a problem hiding this comment.
After recreating the energy grid, convolution_plan_is_valid is never set back to True.
This means every subsequent call to convolution() will unnecessarily recreate the energy grid. This is a performance issue.
There was a problem hiding this comment.
'_upsample_factor',
'_extension_factor',
'_normalize_detailed_balance',
These attributes no longer exist on Convolution- they live inside ConvolutionSettings. The __setattr__ hook will never match these names. They are dead code.
| from easydynamics.utils.utils import Numeric | ||
|
|
||
|
|
||
| class ConvolutionSettings(EasyDynamicsBase): |
There was a problem hiding this comment.
Do we want to export ConvolutionSettings in the convolution __init__ file? Or dynamics __init__?
| upsample_factor: Numeric | None = 5, | ||
| extension_factor: Numeric | None = 0.2, |
There was a problem hiding this comment.
extension_factor and upsample_factor are stored directly without float() conversion, but the setters convert to float(). Consider standardizing.
| # from dataclasses import dataclass | ||
|
|
2 participants
E.g.
Missing a few unit tests, but ready for review