Move index dependent calculation to megacomplexes for speed-up #1175
Conversation
This file contains 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
|
|
Sourcery Code Quality Report❌ Merging this PR will decrease code quality in the affected files by 1.21%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
jsnel
requested changes
Nov 11, 2022
There was a problem hiding this comment.
- examples are not running on CI,
doas example is broken
locally the coherent artifact unit test is also broken (but can't see that in the CI yet)
(can't start proper review until that's fixed)
FAILED glotaran/builtin/megacomplexes/coherent_artifact/test/test_coherent_artifact.py::test_coherent_artifact[none] - assert (34, 4) == (3, 34, 4)
> assert matrix.matrix.shape == (spectral.size, time.size, 4)
E assert (34, 4) == (3, 34, 4)
E At index 0 diff: 34 != 3
E Right contains one more item: 4
E Use -v to get more diff
- my terminal is spammed with deprecation warnings from numba:
numba\core\ir_utils.py:2147: NumbaPendingDeprecationWarning:
Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'centers' of function '_calculate_coherent_artifact_matrix'.
For more information visit https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-reflection-for-list-and-set-types
File "..\pyglotaran\glotaran\builtin\megacomplexes\coherent_artifact\coherent_artifact_megacomplex.py", line 104:
@nb.jit(nopython=True, parallel=True)
def _calculate_coherent_artifact_matrix(
^
warnings.warn(NumbaPendingDeprecationWarning(msg, loc=loc))
numba\core\ir_utils.py:2147: NumbaPendingDeprecationWarning:
Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'widths' of function '_calculate_coherent_artifact_matrix'.
For more information visit https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-reflection-for-list-and-set-types
File "..\pyglotaran\glotaran\builtin\megacomplexes\coherent_artifact\coherent_artifact_megacomplex.py", line 104:
@nb.jit(nopython=True, parallel=True)
def _calculate_coherent_artifact_matrix(
^
warnings.warn(NumbaPendingDeprecationWarning(msg, loc=loc))
numba\core\ir_utils.py:2147: NumbaPendingDeprecationWarning:
Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'centers' of function '__numba_parfor_gufunc_0x1b19cd827a0'.
For more information visit https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-reflection-for-list-and-set-types
File "<string>", line 1:
<source missing, REPL/exec in use?>
warnings.warn(NumbaPendingDeprecationWarning(msg, loc=loc))
numba\core\ir_utils.py:2147: NumbaPendingDeprecationWarning:
Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'widths' of function '__numba_parfor_gufunc_0x1b19cd827a0'.
For more information visit https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-reflection-for-list-and-set-types
File "<string>", line 1:
<source missing, REPL/exec in use?>
jsnel
force-pushed
the
refactor/index_dependency
branch
from
6a99cf6 to
49daf7d
Compare
|
I just benchmarked one of the 'heaviest' example we have, pyglotaran_examples\test\simultaneous_analysis_6d_disp\sim_analysis_script_6d_disp.py and the results are impressive! main: 32.1s Nice @joernweissenborn , I think @ism200 will happy to learn about this! |
Codecov ReportBase: 87.7% // Head: 87.6% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1175 +/- ##
=======================================
- Coverage 87.7% 87.6% -0.2%
=======================================
Files 103 104 +1
Lines 4906 4946 +40
Branches 809 819 +10
=======================================
+ Hits 4306 4335 +29
- Misses 486 493 +7
- Partials 114 118 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Even so b7d0eeb didn't fix the strange CI issue IMHO we should keep it for consistency. |
jsnel
force-pushed
the
refactor/index_dependency
branch
from
b7d0eeb to
818f807
Compare
|
Benchmark is done. Checkout the benchmark result page. Benchmark diff v0.6.0 vs. mainParametrized benchmark signatures: BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)Benchmark diff main vs. PRParametrized benchmark signatures: BenchmarkOptimize.time_optimize(index_dependent, grouped, weight) |
jsnel
changed the title
Adapted to changed Megacomplex API
This prevents and esoteric CI bug
Fix the issue: `TypeError: unsupported operand type(s) for -: 'list' and 'int'`
This causes issues on Mac OS X. Also the fix results in a 2x speedup on Windows in some cases. Also know as fix for "Terminating: Nested parallel kernel launch detected, the workqueue threading layer does not supported nested parallelism. Try the TBB threading layer." Ref insightful comment on GitHub: lmcinnes/umap#665 (comment)
s-weigand
force-pushed
the
refactor/index_dependency
branch
from
d4a78e5 to
26b0101
Compare
Was np.ones, was changed to np.zeros, now changes to np.ones again. It should be document why it should be like that!
Change added: - ♻️ Move index dependent calculation to megacomplexes for speed-up (glotaran#1175) Not that this results in a significant speedup in simple cases with dispersion and/or multiple datasets, up to 4x. For more complex cases there is little to no speedup.
Address some deprecation warnings shown when running the doas example. 2x NumbaPendingDeprecationWarning: - Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'centers' of function '_calculate_coherent_artifact_matrix'. - Encountered the use of a type that is scheduled for deprecation: type 'reflected list' found for argument 'widths' of function '_calculate_coherent_artifact_matrix'. For more information visit https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-reflection-for-list-and-set-types
|
Kudos, SonarCloud Quality Gate passed!
|
jsnel
approved these changes
Nov 20, 2022
jsnel
added a commit
that referenced
this pull request
Nov 28, 2022
* 🩹Fix matrix provider This PR fixes an issue with grouping inadvertently introduced in PR #1175 * 📚 Added change to changelog Since this fixed a bug introduced within the 0.7.0 development cycle (so after 0.6.0 but before 0.7.0) the change is included in a 'hidden' manner (not rendered unless you look at the source code of the MD file)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR changes the way index dependent matrices are calculated. Instead of looping in the engine, the full matrix is now calculated by the megacomplex. This should speed up index dependent calculations.
It also fixed a bug in dampened oscillation, the matrix was initialized with ones instead zeros.Note to reviewers: I needed to touch all megacomplexes. They all need (further) brush up, that will be done in follow-up PR.
Change summary
Checklist