🩹 Fix coherent artifact crash for index dependent models #808

s-weigand · 2021-09-11T14:25:11Z

For details look at #793

This PR is best reviewed per commit.

Change summary

🩹 Fixes crashing of CoherentArtifactMegacomplex.finalize_data for index dependent models
♻️ Refactored test_coherent_artifact to be more readable
🧪 Parametrized test_coherent_artifact to cover index dependent case
♻️ Renamed DatasetModel.index_dependent ->DatasetModel.is_index_dependent (see 🚧 Naming conventions for boolean switches #755)

Checklist

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

Closes issues

closes #793

github-actions · 2021-09-11T14:25:24Z

👈 Launch a binder notebook on branch s-weigand/pyglotaran/fix-choherent-artifact-crash

jsnel

Code reviewed: 👌

codecov · 2021-09-11T15:10:32Z

Codecov Report

Merging #808 (8821699) into staging (9892b6e) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           staging    #808   +/-   ##
=======================================
  Coverage     84.5%   84.5%           
=======================================
  Files           75      75           
  Lines         4198    4201    +3     
  Branches       756     757    +1     
=======================================
+ Hits          3550    3553    +3     
  Misses         515     515           
  Partials       133     133

Impacted Files	Coverage Δ
glotaran/analysis/problem.py	`90.9% <100.0%> (ø)`
glotaran/analysis/problem_grouped.py	`96.0% <100.0%> (ø)`
glotaran/analysis/problem_ungrouped.py	`93.8% <100.0%> (ø)`
glotaran/analysis/simulation.py	`82.9% <100.0%> (ø)`
...coherent_artifact/coherent_artifact_megacomplex.py	`73.5% <100.0%> (+1.5%)`	⬆️
glotaran/model/dataset_model.py	`82.0% <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 9892b6e...8821699. Read the comment docs.

s-weigand · 2021-09-11T15:36:07Z

Now it should be ready.
I'll add the sourcery configuration commit in a different PR.

glotaran/builtin/megacomplexes/coherent_artifact/coherent_artifact_megacomplex.py

…ponse' Ref: glotaran#808 (comment)

joernweissenborn

lgtm

If the dataset_model was index_dependent, CoherentArtifactMegacomplex.finalize_data did crash because it tried cast data of dimensions (global_dimension, model_dimension, "coherent_artifact_order") to the dimensions (model_dimension, "coherent_artifact_order")

(see glotaran#755)

…ponse' Ref: glotaran#808 (comment)

sonarcloud · 2021-09-11T21:10:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sourcery-ai · 2021-09-11T21:12:29Z

Sourcery Code Quality Report

Merging this PR leaves code quality unchanged.

Quality metrics	Before	After	Change
Complexity	6.49 ⭐	6.49 ⭐	0.00
Method Length	68.67 🙂	68.67 🙂	0.00
Working memory	9.68 🙂	9.68 🙂	0.00
Quality	63.27% 🙂	63.27% 🙂	0.00%

Other metrics	Before	After	Change
Lines	1505	1505	0

Changed files	Quality Before	Quality After	Quality Change
glotaran/analysis/problem.py	80.36% ⭐	80.36% ⭐	0.00%
glotaran/analysis/problem_grouped.py	57.20% 🙂	57.20% 🙂	0.00%
glotaran/analysis/problem_ungrouped.py	67.75% 🙂	67.75% 🙂	0.00%
glotaran/analysis/simulation.py	64.20% 🙂	64.20% 🙂	0.00%
glotaran/analysis/test/test_optimization.py	34.60% 😞	34.60% 😞	0.00%

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

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/analysis/test/test_optimization.py	test_optimization	19 😞	509 ⛔	15 😞	25.13% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/analysis/problem_grouped.py	GroupedProblem._append_to_grouped_bag	12 🙂	269 ⛔	23 ⛔	29.09% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/analysis/problem_ungrouped.py	UngroupedProblem._calculate_residual	13 🙂	214 ⛔	12 😞	41.40% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/analysis/problem_grouped.py	GroupedProblem.init_bag	10 🙂	174 😞	16 ⛔	41.86% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/analysis/problem.py	Problem._add_weight	15 🙂	134 😞	11 😞	48.74% 😞	Try splitting into smaller methods. 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!

github-actions · 2021-09-11T21:28:27Z

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

Benchmark diff

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [dc00e6da]       [8821699d]
     <v0.4.0>                   
-        69.2±2ms         49.2±1ms     0.71  BenchmarkOptimize.time_optimize(False, False, False)
-         381±6ms         66.3±4ms     0.17  BenchmarkOptimize.time_optimize(False, False, True)
-        95.8±1ms         81.9±2ms     0.85  BenchmarkOptimize.time_optimize(False, True, False)
          100±3ms         97.6±4ms     0.97  BenchmarkOptimize.time_optimize(False, True, True)
         73.0±3ms         69.4±5ms     0.95  BenchmarkOptimize.time_optimize(True, False, False)
-         395±6ms        81.4±30ms     0.21  BenchmarkOptimize.time_optimize(True, False, True)
         98.6±3ms          102±4ms     1.03  BenchmarkOptimize.time_optimize(True, True, False)
+         104±2ms         123±30ms     1.19  BenchmarkOptimize.time_optimize(True, True, True)
             182M             179M     0.99  IntegrationTwoDatasets.peakmem_create_result
             201M             197M     0.98  IntegrationTwoDatasets.peakmem_optimize
-         303±7ms          245±4ms     0.81  IntegrationTwoDatasets.time_create_result
-      6.52±0.01s        2.12±0.1s     0.32  IntegrationTwoDatasets.time_optimize

* 🩹 Fixed dimension mismatching of CoherentArtifactMegacomplex If the dataset_model was index_dependent, CoherentArtifactMegacomplex.finalize_data did crash because it tried cast data of dimensions (global_dimension, model_dimension, "coherent_artifact_order") to the dimensions (model_dimension, "coherent_artifact_order") * ♻️ Refactored test_coherent_artifact to be more readable * 🧪 Parametrized test_coherent_artifact to cover index dependent case * ♻️ Renamed 'DatasetModel.index_dependent' ->'is_index_dependent' (see #755) * 🔧 Let mypy run normally in pre-commit * 👌 Renamed 'coherent_artifact_concentration' to 'coherent_artifact_response' Ref: #808 (comment)

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

s-weigand requested review from joernweissenborn and jsnel as code owners September 11, 2021 14:25

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

🩹 Fix coherent artifact crash for index dependent models (Sourcery refactored) #809

Closed

s-weigand requested a review from a team as a code owner September 11, 2021 14:55

jsnel approved these changes Sep 11, 2021

View reviewed changes

s-weigand force-pushed the fix-choherent-artifact-crash branch from f7f3b98 to 93223cb Compare September 11, 2021 15:35

joernweissenborn requested changes Sep 11, 2021

View reviewed changes

glotaran/builtin/megacomplexes/coherent_artifact/coherent_artifact_megacomplex.py Outdated Show resolved Hide resolved

s-weigand added a commit to s-weigand/pyglotaran that referenced this pull request Sep 11, 2021

👌 Renamed 'coherent_artifact_concentration' to 'coherent_artifact_res…

08b0aae

…ponse' Ref: glotaran#808 (comment)

s-weigand requested a review from joernweissenborn September 11, 2021 21:02

joernweissenborn approved these changes Sep 11, 2021

View reviewed changes

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

🩹 Fix coherent artifact crash for index dependent models (Sourcery refactored) #810

Closed

s-weigand added 6 commits September 11, 2021 23:09

♻️ Refactored test_coherent_artifact to be more readable

df052c8

🧪 Parametrized test_coherent_artifact to cover index dependent case

7c390ee

♻️ Renamed 'DatasetModel.index_dependent' ->'is_index_dependent'

4d5aebf

(see glotaran#755)

🔧 Let mypy run normally in pre-commit

8ffdcb0

👌 Renamed 'coherent_artifact_concentration' to 'coherent_artifact_res…

8821699

…ponse' Ref: glotaran#808 (comment)

s-weigand force-pushed the fix-choherent-artifact-crash branch from 08b0aae to 8821699 Compare September 11, 2021 21:09

jsnel merged commit ae63eb4 into glotaran:staging Sep 11, 2021

jsnel deleted the fix-choherent-artifact-crash branch September 11, 2021 21:41

jsnel mentioned this pull request Sep 16, 2021

✨✨✨ Megacomplex based models, full models (spectrotemporal), damped oscillations models (DOAS), 🧪 test result validation ✔️ #778

Merged

5 tasks

s-weigand mentioned this pull request Sep 16, 2021

🐛 CoherentArtifactMegacomplex.finalize_data crashes due to wrong dimensions #793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🩹 Fix coherent artifact crash for index dependent models #808

🩹 Fix coherent artifact crash for index dependent models #808

s-weigand commented Sep 11, 2021 •

edited

Loading

github-actions bot commented Sep 11, 2021

jsnel left a comment

codecov bot commented Sep 11, 2021 •

edited

Loading

s-weigand commented Sep 11, 2021

joernweissenborn left a comment

sonarcloud bot commented Sep 11, 2021

sourcery-ai bot commented Sep 11, 2021

github-actions bot commented Sep 11, 2021

🩹 Fix coherent artifact crash for index dependent models #808

🩹 Fix coherent artifact crash for index dependent models #808

Conversation

s-weigand commented Sep 11, 2021 • edited Loading

Change summary

Checklist

Closes issues

github-actions bot commented Sep 11, 2021

jsnel left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2021 • edited Loading

Codecov Report

s-weigand commented Sep 11, 2021

joernweissenborn left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 11, 2021

sourcery-ai bot commented Sep 11, 2021

Sourcery Code Quality Report

Legend and Explanation

github-actions bot commented Sep 11, 2021

s-weigand commented Sep 11, 2021 •

edited

Loading

codecov bot commented Sep 11, 2021 •

edited

Loading