♻️ Refactor/calc doas with irf #839

jsnel · 2021-09-27T00:02:04Z

Refactor and fix calculate_damped_oscillation_matrix_gaussian_irf in damped_oscillation_megacomplex.py

Change summary

🩹 Fix for time-reversed oscillations
♻️ Improve numerical stability of calculation

Checklist

✔️ Passing the tests (mandatory for all PR's)

📚 Adds documentation of the feature

Related issues:

For code related to this feature see:

🔧 Add debug plotting code to evaluate doas matrix calculation Some temporary debug plotting code to evaluate the changes in implementation during the evolution of the code during the refactor.

To accommodate time-reversed oscillations the sqwidth constant is manipulated so that negative rates have a -1 sign causing a flip of the erf when this constant is used in the calculation of 'b'. Co-authored-by: Ivo <ism200@users.noreply.github.com> Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Results of refactoring so far; 2 iterations, same stating values: 1. 🤔 Start - Starting cost: 6.3345e+04 - End cost: 6.3345e+04 - rmse: 9.75e-01 - variable `a` going up to 1e106 - variable `osc` going up to 1e106 2. 🩹 Fix for reversed osciallations - Starting cost: 3.0537e+04 - End cost: 3.0006e+04 - rmse: 7.02e-01 - variable `osc` bounded to [-2, 2] - variable `a` going up to 1e106 3. 👌 More numerically stable and efficient calculation of a and b - Starting cost: 3.0537e+04 - End cost: 3.0006e+04 - rmse: 7.02e-01 - variable `osc` bounded to [-2, 2] - variable `a` bounded to [-1.5, 1.1]

Used in frequency conversion

github-actions · 2021-09-27T00:02:22Z

👈 Launch a binder notebook on branch jsnel/pyglotaran/refactor/calc_doas_with_irf

codecov · 2021-09-27T00:06:27Z

Codecov Report

Merging #839 (9c7cc5a) into main (d4677d2) will increase coverage by 0.0%.
The diff coverage is 83.3%.

@@          Coverage Diff          @@
##            main    #839   +/-   ##
=====================================
  Coverage   84.8%   84.9%           
=====================================
  Files         77      77           
  Lines       4366    4373    +7     
  Branches     787     786    -1     
=====================================
+ Hits        3705    3713    +8     
  Misses       521     521           
+ Partials     140     139    -1

Impacted Files	Coverage Δ
...mped_oscillation/damped_oscillation_megacomplex.py	`85.7% <83.3%> (+2.1%)`	⬆️

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 d4677d2...9c7cc5a. Read the comment docs.

github-actions · 2021-09-27T00:07:11Z

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

Benchmark diff v0.4.1 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [21ba272a]       [9c7cc5a6]
     <v0.4.1>                   
-        66.6±1ms         40.5±2ms     0.61  BenchmarkOptimize.time_optimize(False, False, False)
-         405±5ms         55.2±3ms     0.14  BenchmarkOptimize.time_optimize(False, False, True)
-        88.3±2ms         73.5±3ms     0.83  BenchmarkOptimize.time_optimize(False, True, False)
         93.7±3ms         82.5±5ms    ~0.88  BenchmarkOptimize.time_optimize(False, True, True)
-        64.3±2ms         54.1±2ms     0.84  BenchmarkOptimize.time_optimize(True, False, False)
-         401±6ms        81.0±50ms     0.20  BenchmarkOptimize.time_optimize(True, False, True)
         89.8±2ms         87.9±3ms     0.98  BenchmarkOptimize.time_optimize(True, True, False)
         95.3±4ms         107±50ms    ~1.13  BenchmarkOptimize.time_optimize(True, True, True)
             181M             179M     0.99  IntegrationTwoDatasets.peakmem_create_result
             197M             196M     1.00  IntegrationTwoDatasets.peakmem_optimize
-         268±3ms          215±4ms     0.80  IntegrationTwoDatasets.time_create_result
-      5.23±0.01s       1.87±0.04s     0.36  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [d4677d2b]       [9c7cc5a6]
         41.3±1ms         40.5±2ms     0.98  BenchmarkOptimize.time_optimize(False, False, False)
        54.4±20ms         55.2±3ms     1.02  BenchmarkOptimize.time_optimize(False, False, True)
         70.2±3ms         73.5±3ms     1.05  BenchmarkOptimize.time_optimize(False, True, False)
        86.3±20ms         82.5±5ms     0.96  BenchmarkOptimize.time_optimize(False, True, True)
         54.5±1ms         54.1±2ms     0.99  BenchmarkOptimize.time_optimize(True, False, False)
        85.7±50ms        81.0±50ms     0.94  BenchmarkOptimize.time_optimize(True, False, True)
         89.5±2ms         87.9±3ms     0.98  BenchmarkOptimize.time_optimize(True, True, False)
         105±20ms         107±50ms     1.02  BenchmarkOptimize.time_optimize(True, True, True)
             180M             179M     0.99  IntegrationTwoDatasets.peakmem_create_result
             198M             196M     0.99  IntegrationTwoDatasets.peakmem_optimize
          216±3ms          215±4ms     1.00  IntegrationTwoDatasets.time_create_result
       1.73±0.07s       1.87±0.04s     1.08  IntegrationTwoDatasets.time_optimize

parameter, parameter, parameter

joernweissenborn

lgtm

This code wasn't touched by this PR, but will clean up the code for the future.

sourcery-ai · 2021-09-30T12:14:38Z

Sourcery Code Quality Report

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

Quality metrics	Before	After	Change
Complexity	4.26 ⭐	3.15 ⭐	-1.11 👍
Method Length	127.17 😞	150.83 😞	23.66 👎
Working memory	12.26 😞	13.89 😞	1.63 👎
Quality	53.96% 🙂	50.35% 🙂	-3.61% 👎

Other metrics	Before	After	Change
Lines	162	217	55

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/megacomplexes/damped_oscillation/damped_oscillation_megacomplex.py	53.96% 🙂	50.35% 🙂	-3.61% 👎

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

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/builtin/megacomplexes/damped_oscillation/damped_oscillation_megacomplex.py	DampedOscillationMegacomplex.finalize_data	6 ⭐	324 ⛔	12 😞	42.93% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/damped_oscillation/damped_oscillation_megacomplex.py	calculate_damped_oscillation_matrix_gaussian_irf	0 ⭐	248 ⛔	18 ⛔	44.53% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/damped_oscillation/damped_oscillation_megacomplex.py	DampedOscillationMegacomplex.calculate_matrix	4 ⭐	193 😞	16 ⛔	45.97% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/megacomplexes/damped_oscillation/damped_oscillation_megacomplex.py	DampedOscillationMegacomplex.ensure_oscillation_parameter	2 ⭐	62 🙂	11 😞	70.70% 🙂	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-09-30T12:15:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

joernweissenborn

Lgtm

jsnel and others added 6 commits September 27, 2021 00:31

♻️ Start of refactor calculate_damped_oscillation_matrix_gaussian_irf

97b0105

🔧 Add debug plotting code to evaluate doas matrix calculation Some temporary debug plotting code to evaluate the changes in implementation during the evolution of the code during the refactor.

🧹 Clean up debug plotting code

b86b02a

📚 Add comment explaining origin of 0.03 scaling factor

a1a4013

Used in frequency conversion

📚 Add docs for calculate_damped_oscillation_matrix_gaussian_irf

f9c97ac

jsnel requested a review from joernweissenborn as a code owner September 27, 2021 00:02

jsnel added 2 commits September 27, 2021 02:10

👌 Fix typo

fde2011

parameter, parameter, parameter

📚 Improved code comments

4d4eb89

sourcery-ai bot mentioned this pull request Sep 28, 2021

♻️ Refactor/calc doas with irf (Sourcery refactored) #840

Closed

s-weigand added this to the v0.5.0 milestone Sep 30, 2021

joernweissenborn previously approved these changes Sep 30, 2021

View reviewed changes

♻️ Refactored by Sourcery

9c7cc5a

This code wasn't touched by this PR, but will clean up the code for the future.

s-weigand dismissed joernweissenborn’s stale review via 9c7cc5a September 30, 2021 12:14

joernweissenborn approved these changes Sep 30, 2021

View reviewed changes

s-weigand merged commit 38cdb71 into glotaran:main Sep 30, 2021

s-weigand deleted the refactor/calc_doas_with_irf branch September 30, 2021 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Refactor/calc doas with irf #839

♻️ Refactor/calc doas with irf #839

jsnel commented Sep 27, 2021 •

edited

Loading

github-actions bot commented Sep 27, 2021

codecov bot commented Sep 27, 2021 •

edited

Loading

github-actions bot commented Sep 27, 2021 •

edited

Loading

joernweissenborn left a comment

sourcery-ai bot commented Sep 30, 2021

sonarqubecloud bot commented Sep 30, 2021

joernweissenborn left a comment

♻️ Refactor/calc doas with irf #839

♻️ Refactor/calc doas with irf #839

Conversation

jsnel commented Sep 27, 2021 • edited Loading

Change summary

Checklist

Related issues:

github-actions bot commented Sep 27, 2021

codecov bot commented Sep 27, 2021 • edited Loading

Codecov Report

github-actions bot commented Sep 27, 2021 • edited Loading

joernweissenborn left a comment

Choose a reason for hiding this comment

sourcery-ai bot commented Sep 30, 2021

Sourcery Code Quality Report

Legend and Explanation

sonarqubecloud bot commented Sep 30, 2021

joernweissenborn left a comment

Choose a reason for hiding this comment

jsnel commented Sep 27, 2021 •

edited

Loading

codecov bot commented Sep 27, 2021 •

edited

Loading

github-actions bot commented Sep 27, 2021 •

edited

Loading