Fix several issues with KineticImage model #612

joernweissenborn · 2021-03-26T13:16:27Z

codecov · 2021-03-26T13:19:29Z

Codecov Report

Merging #612 (72c0746) into main (0d41d63) will increase coverage by 0.0%.
The diff coverage is 75.0%.

@@          Coverage Diff          @@
##            main    #612   +/-   ##
=====================================
  Coverage   77.7%   77.8%           
=====================================
  Files         63      63           
  Lines       3531    3537    +6     
  Branches     657     658    +1     
=====================================
+ Hits        2745    2753    +8     
+ Misses       661     655    -6     
- Partials     125     129    +4

Impacted Files	Coverage Δ
glotaran/builtin/models/kinetic_image/irf.py	`73.8% <ø> (ø)`
glotaran/model/dataset_descriptor.py	`100.0% <ø> (ø)`
...iltin/models/kinetic_image/kinetic_image_matrix.py	`52.1% <40.0%> (-1.8%)`	⬇️
...iltin/models/kinetic_image/kinetic_image_result.py	`80.3% <80.0%> (ø)`
...models/kinetic_spectrum/kinetic_spectrum_result.py	`80.5% <80.0%> (ø)`
.../kinetic_image/kinetic_image_dataset_descriptor.py	`94.1% <88.8%> (-5.9%)`	⬇️
...ltin/models/kinetic_image/initial_concentration.py	`100.0% <100.0%> (ø)`
glotaran/builtin/models/kinetic_image/k_matrix.py	`79.6% <100.0%> (+6.1%)`	⬆️
.../models/kinetic_image/kinetic_image_megacomplex.py	`72.2% <100.0%> (-4.3%)`	⬇️
... and 1 more

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 0d41d63...72c0746. Read the comment docs.

lgtm-com · 2021-03-26T13:23:48Z

This pull request introduces 1 alert when merging c42a9c1 into a7bc028 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

s-weigand

I found some minor consistency and overcomplexity issues.
@jsnel needs to decide on the actual impact of the changes.

glotaran/builtin/models/kinetic_image/test/test_k_matrix.py

glotaran/builtin/models/kinetic_image/k_matrix.py

glotaran/builtin/models/kinetic_image/kinetic_image_dataset_descriptor.py

lgtm-com · 2021-03-27T08:37:17Z

This pull request introduces 1 alert when merging bd79d60 into 015d5c7 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

glotaran/builtin/models/kinetic_image/irf.py

sonarcloud · 2021-03-27T16:23:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

lgtm-com · 2021-03-27T16:31:36Z

This pull request introduces 1 alert when merging f73d244 into 015d5c7 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

lgtm-com · 2021-03-27T18:38:44Z

This pull request introduces 1 alert when merging 3485ea1 into 015d5c7 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

lgtm-com · 2021-03-27T18:46:06Z

This pull request introduces 1 alert when merging 3462c52 into 015d5c7 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

s-weigand

Found another small issue, LGTM was justifiedly complaining about.

glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py

…. Solves glotaran#604

…left one. Solves glotaran#605

Megacomplex scales are now a property of the dataset_descriptor. They are defined per dataset per megacomplex. Fixes glotaran#606

Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

…scriptor.py Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Accessing the matrix by key returns the parameter stored at that key, whereas in the test the full_label is used to test if combining worked.

Just enough indentation

lgtm-com · 2021-03-28T22:24:34Z

This pull request introduces 1 alert when merging 952dafb into 0d41d63 - view on LGTM.com

new alerts:

1 for Nested loops with same variable

Rename variables to be more clear

jsnel

LGTM ✨

s-weigand

Besides a nitpicky thing with a docstring LGTM

s-weigand · 2021-03-28T22:53:31Z

glotaran/builtin/models/kinetic_image/k_matrix.py

+        When combining k-matrices a and b (a.combine(b)),
+        entries in a will overwrite by corresponding entries in b.


Suggested change

When combining k-matrices a and b (a.combine(b)),

entries in a will overwrite by corresponding entries in b.

When combining k-matrices a and b (a.combine(b)),

entries in a will overwrite by corresponding entries in b.

Sorry for being nitpicky, but this would be one less error to fix when we at some point are able to add pydocstyle.

Internal API only. User exposed attributes already had correct spelling of associated

sourcery-ai · 2021-03-28T23:11:20Z

Sourcery Code Quality Report

✅ Merging this PR will increase code quality in the affected files by 0.52%.

Quality metrics	Before	After	Change
Complexity	5.23 ⭐	5.20 ⭐	-0.03 👍
Method Length	73.52 🙂	72.67 🙂	-0.85 👍
Working memory	10.35 😞	10.20 😞	-0.15 👍
Quality	63.51% 🙂	64.03% 🙂	0.52% 👍

Other metrics	Before	After	Change
Lines	1149	1195	46

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/io/yml/test/test_model_parser_kinetic.py	79.22% ⭐	79.15% ⭐	-0.07% 👎
glotaran/builtin/models/kinetic_image/initial_concentration.py	68.36% 🙂	83.46% ⭐	15.10% 👍
glotaran/builtin/models/kinetic_image/irf.py	52.29% 🙂	52.29% 🙂	0.00%
glotaran/builtin/models/kinetic_image/k_matrix.py	75.31% ⭐	75.31% ⭐	0.00%
glotaran/builtin/models/kinetic_image/kinetic_image_dataset_descriptor.py	93.95% ⭐	88.12% ⭐	-5.83% 👎
glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py	48.30% 😞	46.86% 😞	-1.44% 👎
glotaran/builtin/models/kinetic_image/kinetic_image_megacomplex.py	86.76% ⭐	88.36% ⭐	1.60% 👍
glotaran/builtin/models/kinetic_image/kinetic_image_result.py	45.36% 😞	45.36% 😞	0.00%
glotaran/builtin/models/kinetic_image/test/test_k_matrix.py	54.12% 🙂	58.90% 🙂	4.78% 👍
glotaran/builtin/models/kinetic_spectrum/kinetic_spectrum_result.py	28.10% 😞	28.10% 😞	0.00%
glotaran/model/dataset_descriptor.py	78.18% ⭐	74.64% 🙂	-3.54% 👎
glotaran/model/test/test_model.py	71.60% 🙂	71.58% 🙂	-0.02% 👎

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

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py	kinetic_matrix	25 😞	235 ⛔	15 😞	27.04% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/models/kinetic_spectrum/kinetic_spectrum_result.py	finalize_kinetic_spectrum_result	24 😞	250 ⛔	14 😞	28.10% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/models/kinetic_image/kinetic_image_result.py	retrieve_decay_associated_data	10 🙂	326 ⛔	20 ⛔	30.46% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py	calculate_kinetic_matrix_gaussian_irf	10 🙂	177 😞	19 ⛔	38.97% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/models/kinetic_image/test/test_k_matrix.py	test_unibranched	0 ⭐	206 ⛔	17 ⛔	47.98% 😞	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!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

sonarcloud · 2021-03-28T23:11:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jsnel

LGTM ✨

joernweissenborn requested a review from jsnel March 26, 2021 13:16

joernweissenborn requested a review from a team as a code owner March 26, 2021 13:16

s-weigand reviewed Mar 26, 2021

View reviewed changes

jsnel force-pushed the joern/0.4 branch from c42a9c1 to bd79d60 Compare March 27, 2021 08:25

jsnel reviewed Mar 27, 2021

View reviewed changes

glotaran/builtin/models/kinetic_image/irf.py Show resolved Hide resolved

jsnel changed the title ~~Fix serval issues with KineticImage model~~ Fix several issues with KineticImage model Mar 27, 2021

s-weigand reviewed Mar 27, 2021

View reviewed changes

glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py Outdated Show resolved Hide resolved

glotaran/builtin/models/kinetic_image/kinetic_image_matrix.py Outdated Show resolved Hide resolved

joernweissenborn and others added 10 commits March 29, 2021 00:14

Updated gitignore for virtualenv

9fec8a5

Made Irf.normalize true by default and only normalize matrix when set…

dc0bdeb

…. Solves glotaran#604

When combinging k matrices, the right hand matrix now overwrites the …

a7bdee3

…left one. Solves glotaran#605

Changed megacomplex scale application and definition.

12817b1

Megacomplex scales are now a property of the dataset_descriptor. They are defined per dataset per megacomplex. Fixes glotaran#606

Update glotaran/builtin/models/kinetic_image/test/test_k_matrix.py

ac1b0a1

Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Update glotaran/builtin/models/kinetic_image/kinetic_image_dataset_de…

0ed8698

…scriptor.py Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Update glotaran/builtin/models/kinetic_image/kinetic_image_dataset_de…

f9195e8

…scriptor.py Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Update glotaran/builtin/models/kinetic_image/kinetic_image_dataset_de…

f82cccd

…scriptor.py Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com>

Fix unit test for combining k matrices

94ba121

Accessing the matrix by key returns the parameter stored at that key, whereas in the test the full_label is used to test if combining worked.

Fix formatting

952dafb

Just enough indentation

jsnel force-pushed the joern/0.4 branch from 3462c52 to 952dafb Compare March 28, 2021 22:17

jsnel self-requested a review March 28, 2021 22:17

Avoid using same variable name in nested for loops

bfa856c

Rename variables to be more clear

jsnel requested a review from s-weigand March 28, 2021 22:32

jsnel previously approved these changes Mar 28, 2021

View reviewed changes

This was linked to issues Mar 28, 2021

Combine KMatrices: change order of override #605

Closed

Megacomplex scale must be defined per dataset and per megacomplex #606

Closed

s-weigand previously approved these changes Mar 28, 2021

View reviewed changes

jsnel added 2 commits March 29, 2021 01:09

Fix typo associated in kinetic_*_result

cd47fae

Internal API only. User exposed attributes already had correct spelling of associated

Update docstring for KMatrix.combine

72c0746

jsnel dismissed stale reviews from s-weigand and themself via 72c0746 March 28, 2021 23:11

jsnel force-pushed the joern/0.4 branch from 4139280 to 72c0746 Compare March 28, 2021 23:11

jsnel approved these changes Mar 28, 2021

View reviewed changes

jsnel merged commit b188047 into glotaran:main Mar 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several issues with KineticImage model #612

Fix several issues with KineticImage model #612

joernweissenborn commented Mar 26, 2021

codecov bot commented Mar 26, 2021 •

edited

Loading

lgtm-com bot commented Mar 26, 2021

s-weigand left a comment

lgtm-com bot commented Mar 27, 2021

sonarcloud bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

s-weigand left a comment

lgtm-com bot commented Mar 28, 2021

jsnel left a comment

s-weigand left a comment

s-weigand Mar 28, 2021

sourcery-ai bot commented Mar 28, 2021

sonarcloud bot commented Mar 28, 2021

jsnel left a comment

		When combining k-matrices a and b (a.combine(b)),
		entries in a will overwrite by corresponding entries in b.

Fix several issues with KineticImage model #612

Fix several issues with KineticImage model #612

Conversation

joernweissenborn commented Mar 26, 2021

codecov bot commented Mar 26, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Mar 26, 2021

s-weigand left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 27, 2021

sonarcloud bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

lgtm-com bot commented Mar 27, 2021

s-weigand left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 28, 2021

jsnel left a comment

Choose a reason for hiding this comment

s-weigand left a comment

Choose a reason for hiding this comment

s-weigand Mar 28, 2021

Choose a reason for hiding this comment

sourcery-ai bot commented Mar 28, 2021

Sourcery Code Quality Report

Legend and Explanation

sonarcloud bot commented Mar 28, 2021

jsnel left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2021 •

edited

Loading