🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927

joernweissenborn · 2021-12-03T12:51:36Z

Change summary

Use normalized Irf in creation of resulting A matrix and DAS
Bugfix also back-ported to v0.4 maintenance branch in 🚀🩹 v0.4.2 release preparation and bugfix backport for result consistency checking purposes #935

Checklist

✔️ Passing the tests (mandatory for all PR's)
👌 Closes issue (mandatory for ✨ feature and 🩹 bug fix PR's)

Closes issues

Closes #926, #932

github-actions · 2021-12-03T12:51:52Z

👈 Launch a binder notebook on branch joernweissenborn/pyglotaran/Hotfix/use_normalized_irf_in_result

github-actions · 2021-12-03T12:55:56Z

Benchmark is done. Checkout the benchmark result page.
Benchmark differences below 5% might be due to CI noise.

Benchmark diff v0.5.0 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [89046e0a]       [2d840717]
     <v0.5.0>                   
       52.9±0.3ms       52.5±0.3ms     0.99  BenchmarkOptimize.time_optimize(False, False, False)
       56.4±0.9ms       56.0±0.4ms     0.99  BenchmarkOptimize.time_optimize(False, False, True)
       52.8±0.2ms       52.7±0.3ms     1.00  BenchmarkOptimize.time_optimize(False, True, False)
         59.1±9ms         57.1±9ms     0.97  BenchmarkOptimize.time_optimize(False, True, True)
       64.2±0.2ms       64.3±0.4ms     1.00  BenchmarkOptimize.time_optimize(True, False, False)
         68.2±9ms        68.4±40ms     1.00  BenchmarkOptimize.time_optimize(True, False, True)
       64.6±0.3ms       64.3±0.3ms     1.00  BenchmarkOptimize.time_optimize(True, True, False)
        70.7±40ms         126±30ms    ~1.78  BenchmarkOptimize.time_optimize(True, True, True)
             189M             193M     1.02  IntegrationTwoDatasets.peakmem_optimize
       1.80±0.07s       1.77±0.05s     0.98  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [3c435e15]       [2d840717]
       52.5±0.5ms       52.5±0.3ms     1.00  BenchmarkOptimize.time_optimize(False, False, False)
        58.3±40ms       56.0±0.4ms     0.96  BenchmarkOptimize.time_optimize(False, False, True)
       52.4±0.5ms       52.7±0.3ms     1.01  BenchmarkOptimize.time_optimize(False, True, False)
       56.6±0.6ms         57.1±9ms     1.01  BenchmarkOptimize.time_optimize(False, True, True)
       63.2±0.4ms       64.3±0.4ms     1.02  BenchmarkOptimize.time_optimize(True, False, False)
       67.0±0.5ms        68.4±40ms     1.02  BenchmarkOptimize.time_optimize(True, False, True)
       63.2±0.7ms       64.3±0.3ms     1.02  BenchmarkOptimize.time_optimize(True, True, False)
        67.1±40ms         126±30ms    ~1.88  BenchmarkOptimize.time_optimize(True, True, True)
             190M             193M     1.02  IntegrationTwoDatasets.peakmem_optimize
       1.75±0.03s       1.77±0.05s     1.01  IntegrationTwoDatasets.time_optimize

codecov · 2021-12-03T12:56:39Z

Codecov Report

Merging #927 (2d84071) into main (3c435e1) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #927   +/-   ##
=====================================
  Coverage   85.1%   85.1%           
=====================================
  Files         85      85           
  Lines       4799    4799           
  Branches     921     921           
=====================================
  Hits        4085    4085           
  Misses       562     562           
  Partials     152     152

Impacted Files	Coverage Δ
glotaran/builtin/megacomplexes/decay/util.py	`73.1% <100.0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c435e1...2d84071. Read the comment docs.

jsnel · 2021-12-03T23:12:24Z

Before putting in this hot-fix, @ism200 has requested we dig a little deeper (see #932 ) to ensure that the normalization is correct under all conditions (in particular for the simple models introduced in #860 ).

Also, when making any changes to a_matrix we might also want to take into account its use in a_matrix_as_markdown in k_matrix.py in addition to decay_megacomplex.py and builtin\megacomplexes\decay\util.py

jsnel

Changed requested following outcome of investigation in #932 .

Backport of bugfix glotaran#927 discovered in PR glotaran#860 It was [found](glotaran#860 (comment)) in glotaran#860 that the a_matrix when calculated for result saving was calculated without normalized initial_concentrations while the calculation for the optimization was (correctly) using the normalized version. Fixing this bug in glotaran#927 then breaks result consistency checking for some case studies, thus requiring backporting of this bugfix to the 0.4 maintenance branch which we continue to use for result/consistency checking.

- 🩹🚧 Backport of bugfix glotaran#927 discovered in PR glotaran#860 related to initial_concentration normalization when saving results (glotaran#934).

- 🩹🚧 Backport of bugfix glotaran#927 discovered in PR glotaran#860 related to initial_concentration normalization when saving results (glotaran#935).

…ncy checking purposes (#935) * 🩹🚧 Backport of bugfix Backport of bugfix #927 discovered in PR #860 It was [found](#860 (comment)) in #860 that the a_matrix when calculated for result saving was calculated without normalized initial_concentrations while the calculation for the optimization was (correctly) using the normalized version. Fixing this bug in #927 then breaks result consistency checking for some case studies, thus requiring backporting of this bugfix to the 0.4 maintenance branch which we continue to use for result/consistency checking. * 🚀🚧 v0.4.2 Maintenance release - 🩹🚧 Backport of bugfix #927 discovered in PR #860 related to initial_concentration normalization when saving results (#935). * 🚇🚧 Updated 'gold standard' result comparison reference Updated pyglotaran-examples result-comparison branch for pyglotaran v0.4.2 Note: comments in PR #936 provide explanation for a small test deviation in the results comparison check for this PR.

* 🚇Update test_result_consistency Update float_resolution for comparison of spectral values. Should resolve issues with result comparison signaled in #927 * 🚇👌 Further refinement of test_result_consistency Refine calculation of epsilon for residual check Should resolve issues with result comparison signaled in #896 * 🚇👌 Failing tests now also show the tolerances used for the comparison

🚇Update test_result_consistency (glotaran#936) * 🚇Update test_result_consistency Update float_resolution for comparison of spectral values. Should resolve issues with result comparison signaled in glotaran#927 * 🚇👌 Further refinement of test_result_consistency Refine calculation of epsilon for residual check Should resolve issues with result comparison signaled in glotaran#896 * 🚇👌 Failing tests now also show the tolerances used for the comparison d2bff4

…938) * 🚧 Forward port of #936 test_result_consistency 🚇Update test_result_consistency (#936) * 🚇Update test_result_consistency Update float_resolution for comparison of spectral values. Should resolve issues with result comparison signaled in #927 * 🚇👌 Further refinement of test_result_consistency Refine calculation of epsilon for residual check Should resolve issues with result comparison signaled in #896 * 🚇👌 Failing tests now also show the tolerances used for the comparison d2bff4 * 📚 Forward port changelog from v0.4 maintenance branch

sourcery-ai · 2021-12-12T02:53:51Z

Sourcery Code Quality Report

❌ Merging this PR will decrease code quality in the affected files by 0.03%.

Quality metrics	Before	After	Change
Complexity	4.45 ⭐	4.45 ⭐	0.00
Method Length	96.33 🙂	96.56 🙂	0.23 👎
Working memory	14.09 😞	14.09 😞	0.00
Quality	53.83% 🙂	53.80% 🙂	-0.03% 👎

Other metrics	Before	After	Change
Lines	220	220	0

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/megacomplexes/decay/util.py	53.83% 🙂	53.80% 🙂	-0.03% 👎

Here are some functions in these files that still need a tune-up:

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/builtin/megacomplexes/decay/util.py	calculate_decay_matrix_gaussian_irf	11 🙂	183 😞	18 ⛔	38.30% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/decay/util.py	retrieve_decay_associated_data	1 ⭐	262 ⛔	17 ⛔	43.71% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/decay/util.py	retrieve_species_associated_data	6 ⭐	109 🙂	14 😞	55.24% 🙂	Extract out complex expressions
glotaran/builtin/megacomplexes/decay/util.py	retrieve_irf	6 ⭐	132 😞	10 😞	58.99% 🙂	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/decay/util.py	decay_matrix_implementation	6 ⭐	68 🙂	15 😞	59.85% 🙂	Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

⭐ excellent
🙂 good
😞 poor
⛔ very poor

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!

sonarqubecloud · 2021-12-12T02:54:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jsnel

LGTM 👍

- 🩹🚧 Backport of bugfix 🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927 - 📚 Updated Readme (PyPI badge link, Discord URL, DOI) - 📚 Updated Changelog to reflect changes - Follow up on: 🚀🩹 v0.4.2 release preparation and bugfix backport for result consistency checking purposes #935).

⬆️ Update version number to 0.5.1 Prepare patch release fixing two bugs: - 🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex (glotaran#927) - 🩹 Fix save_result crashes on Windows if input data are on a different drive than result (glotaran#931)

⬆️ Update version number to 0.5.1 Patch release fixing two bugs: - 🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex (#927) - 🩹 Fix save_result crashes on Windows if input data are on a different drive than result (#931)

joernweissenborn requested a review from jsnel as a code owner December 3, 2021 12:51

joernweissenborn mentioned this pull request Dec 3, 2021

✨ Add simple decay megacomplexes #860

Merged

4 tasks

jsnel mentioned this pull request Dec 3, 2021

🐛 Inconsistent use of initial_concentration normalization w.r.t a_matrix calculation #932

Closed

jsnel requested changes Dec 3, 2021

View reviewed changes

jsnel added a commit to jsnel/pyglotaran that referenced this pull request Dec 11, 2021

🚀🚧 v0.4.2 Maintenance release

8677c65

- 🩹🚧 Backport of bugfix glotaran#927 discovered in PR glotaran#860 related to initial_concentration normalization when saving results (glotaran#934).

jsnel mentioned this pull request Dec 11, 2021

🚀🩹 v0.4.2 release preparation and bugfix backport for result consistency checking purposes #935

Merged

4 tasks

jsnel added a commit to jsnel/pyglotaran that referenced this pull request Dec 11, 2021

🚀🚧 v0.4.2 Maintenance release

a83bcb3

- 🩹🚧 Backport of bugfix glotaran#927 discovered in PR glotaran#860 related to initial_concentration normalization when saving results (glotaran#935).

s-weigand force-pushed the Hotfix/use_normalized_irf_in_result branch from d34e844 to 9a97204 Compare December 12, 2021 01:14

jsnel mentioned this pull request Dec 12, 2021

🚧 Forward port Improve result comparison workflow and v0.4 changelog #938

Merged

2 tasks

Use normalized irf in decay result creation.

2d84071

jsnel force-pushed the Hotfix/use_normalized_irf_in_result branch from 9a97204 to 2d84071 Compare December 12, 2021 02:53

jsnel approved these changes Dec 12, 2021

View reviewed changes

jsnel changed the title ~~Hotfix: Use normalized irf in decay result creation.~~ 🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex Dec 12, 2021

jsnel linked an issue Dec 12, 2021 that may be closed by this pull request

🐛 Inconsistent use of initial_concentration normalization w.r.t a_matrix calculation #932

Closed

jsnel merged commit 4a44404 into glotaran:main Dec 12, 2021

jsnel mentioned this pull request Dec 31, 2021

Prepare 0.4.2 maintenance release #944

Merged

1 task

jsnel mentioned this pull request Dec 31, 2021

🚀 Prepare v0.5.1 bugfix release #945

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927

🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927

joernweissenborn commented Dec 3, 2021 •

edited by jsnel

Loading

github-actions bot commented Dec 3, 2021

github-actions bot commented Dec 3, 2021 •

edited

Loading

codecov bot commented Dec 3, 2021 •

edited

Loading

jsnel commented Dec 3, 2021

jsnel left a comment

sourcery-ai bot commented Dec 12, 2021

sonarqubecloud bot commented Dec 12, 2021

jsnel left a comment

🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927

🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927

Conversation

joernweissenborn commented Dec 3, 2021 • edited by jsnel Loading

Change summary

Checklist

Closes issues

github-actions bot commented Dec 3, 2021

github-actions bot commented Dec 3, 2021 • edited Loading

codecov bot commented Dec 3, 2021 • edited Loading

Codecov Report

jsnel commented Dec 3, 2021

jsnel left a comment

Choose a reason for hiding this comment

sourcery-ai bot commented Dec 12, 2021

Sourcery Code Quality Report

Legend and Explanation

sonarqubecloud bot commented Dec 12, 2021

jsnel left a comment

Choose a reason for hiding this comment

joernweissenborn commented Dec 3, 2021 •

edited by jsnel

Loading

github-actions bot commented Dec 3, 2021 •

edited

Loading

codecov bot commented Dec 3, 2021 •

edited

Loading