🩹 False positive model validation fail when combining multiple default megacomplexes #797

s-weigand · 2021-08-29T13:45:49Z

This PR is just preventing the false-negative validation and not fixing the root cause of the problem.

Change summary

Fix error formatting (missing f in format string)
Improves typing
Adds unittests for DatasetModel.ensure_unique_megacomplexes
Fixes missing report when megacomplex isn't defined
Fixes megacompley type being None
Refactored DatasetModel.ensure_unique_megacomplexes to use counter for uniqueness checking

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 #796

github-actions · 2021-08-29T13:46:02Z

👈 Launch a binder notebook on branch s-weigand/pyglotaran/fix-missing-f-in-f-string-error-format

codecov · 2021-08-29T13:49:56Z

Codecov Report

Merging #797 (dee11fc) into staging (70a1205) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           staging    #797   +/-   ##
=======================================
  Coverage     84.4%   84.4%           
=======================================
  Files           75      75           
  Lines         4196    4195    -1     
  Branches       758     755    -3     
=======================================
+ Hits          3542    3544    +2     
+ Misses         520     518    -2     
+ Partials       134     133    -1

Impacted Files	Coverage Δ
glotaran/model/dataset_model.py	`82.0% <100.0%> (+1.6%)`	⬆️
glotaran/model/megacomplex.py	`98.5% <100.0%> (+1.4%)`	⬆️

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 70a1205...dee11fc. Read the comment docs.

jsnel

LGTM!

joernweissenborn

lgtm

…less

Compatibility with legacy (0.4.0) model specs and combining multiple megacomplexes

And improved typing. Since 'DatasetModel' is never instantiated from the class definition, but 'create_dataset_model_type' creates a new class in a closure each time a new instance is needed, values across instances won't be overwritten.

@joernweissenborn

After a personal request of @joernweissenborn I removed the class attributes again and moved them to a pyi file

Since it is already reported as 'Missing Model Item'

sourcery-ai · 2021-09-03T20:23:29Z

Sourcery Code Quality Report

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

Quality metrics	Before	After	Change
Complexity	5.83 ⭐	5.80 ⭐	-0.03 👍
Method Length	32.08 ⭐	32.75 ⭐	0.67 👎
Working memory	7.45 🙂	7.44 🙂	-0.01 👍
Quality	72.79% 🙂	72.53% 🙂	-0.26% 👎

Other metrics	Before	After	Change
Lines	133	135	2

Changed files	Quality Before	Quality After	Quality Change
glotaran/model/megacomplex.py	72.79% 🙂	72.53% 🙂	-0.26% 👎

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

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/model/megacomplex.py	_add_model_items_to_properties	11 🙂	96 🙂	12 😞	55.31% 🙂	Extract out complex expressions
glotaran/model/megacomplex.py	megacomplex	9 🙂	136 😞	9 🙂	57.74% 🙂	Try splitting into smaller methods

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!

sonarcloud · 2021-09-03T20:23:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2021-09-03T20:26:30Z

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]       [dee11fc8]
     <v0.4.0>                   
-        68.1±1ms         46.3±1ms     0.68  BenchmarkOptimize.time_optimize(False, False, False)
-        382±10ms         60.8±2ms     0.16  BenchmarkOptimize.time_optimize(False, False, True)
-      91.6±0.8ms         80.2±1ms     0.88  BenchmarkOptimize.time_optimize(False, True, False)
         91.4±2ms         88.0±3ms     0.96  BenchmarkOptimize.time_optimize(False, True, True)
         65.7±1ms         61.8±3ms     0.94  BenchmarkOptimize.time_optimize(True, False, False)
-         385±5ms        71.2±30ms     0.19  BenchmarkOptimize.time_optimize(True, False, True)
         95.7±2ms         94.5±4ms     0.99  BenchmarkOptimize.time_optimize(True, True, False)
         97.5±2ms         105±10ms     1.08  BenchmarkOptimize.time_optimize(True, True, True)
             180M             181M     1.01  IntegrationTwoDatasets.peakmem_create_result
             199M             199M     1.00  IntegrationTwoDatasets.peakmem_optimize
-        291±10ms          221±5ms     0.76  IntegrationTwoDatasets.time_create_result
-      5.85±0.01s       1.52±0.05s     0.26  IntegrationTwoDatasets.time_optimize

@joernweissenborn

…t megacomplexes (#797) * 🩹 Fixed missing 'f' in front of f-string which rendered the error useless * 🩹 Deactivate instance check if megacomplex type is None Compatibility with legacy (0.4.0) model specs and combining multiple megacomplexes * 🩹 Fixed usage of 'glotaran_unique' in 'ensure_unique_megacomplexes' * 👌 Improved typing * 🧪 Added unittests for 'ensure_unique_megacomplexes' * 🩹 Fixed missing megacomplex definition not being reported as error * 🩹 Fixed megacomplex type being None * ♻️ Refactored 'ensure_unique_megacomplexes' using Counter and improved typing. Since 'DatasetModel' is never instantiated from the class definition, but 'create_dataset_model_type' creates a new class in a closure each time a new instance is needed, values across instances won't be overwritten. * ♻️ Moved typing information to stub file After a personal request of @joernweissenborn I removed the class attributes again and moved them to a pyi file * 🧹 Removed warning about missing megacomplex definition Since it is already reported as 'Missing Model Item'

s-weigand added Type: Serious Bug Crashes, Broken code, Security Issues Type: Bug Minor issues, non-crashing bug, slowdowns labels Aug 29, 2021

s-weigand requested review from joernweissenborn and jsnel as code owners August 29, 2021 13:45

s-weigand changed the title ~~🩹 Partial hotfix for #796~~ 🩹 False positive model validation fail when combining multiple default megacomplexes Aug 29, 2021

s-weigand force-pushed the fix-missing-f-in-f-string-error-format branch 2 times, most recently from 0719076 to a80b61e Compare August 29, 2021 20:07

jsnel approved these changes Sep 1, 2021

View reviewed changes

joernweissenborn approved these changes Sep 1, 2021

View reviewed changes

s-weigand added 10 commits September 3, 2021 22:21

🩹 Fixed missing 'f' in front of f-string which rendered the error use…

18444f9

…less

🩹 Deactivate instance check if megcomplex type is None

3093737

Compatibility with legacy (0.4.0) model specs and combining multiple megacomplexes

🩹 Fixed usage of 'glotaran_unique' in 'ensure_unique_megacomplexes'

37cd72b

👌 Improved typing

8452dcf

🧪 Added unittests for 'ensure_unique_megacomplexes'

4a6de15

🩹 Fixed missing megacomplex definition not being reported as error

5a64aa1

🩹 Fixed megacomplex type being None

a8e86e9

♻️ Moved typing information to stub file

bdcd192

After a personal request of @joernweissenborn I removed the class attributes again and moved them to a pyi file

🧹 Removed warning about missing megacomplex definition

dee11fc

Since it is already reported as 'Missing Model Item'

jsnel force-pushed the fix-missing-f-in-f-string-error-format branch from a80b61e to dee11fc Compare September 3, 2021 20:22

jsnel merged commit fe7d75c into glotaran:staging Sep 3, 2021

jsnel deleted the fix-missing-f-in-f-string-error-format branch September 3, 2021 20:31

This was referenced Sep 8, 2021

🐛 False positive model validation fail when combining multiple default megacomplexes #796

Closed

🩹 Fix irf location selection for irfs with multiple components glotaran/pyglotaran-extras#32

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🩹 False positive model validation fail when combining multiple default megacomplexes #797

🩹 False positive model validation fail when combining multiple default megacomplexes #797

s-weigand commented Aug 29, 2021 •

edited

Loading

github-actions bot commented Aug 29, 2021

codecov bot commented Aug 29, 2021 •

edited

Loading

jsnel left a comment

joernweissenborn left a comment

sourcery-ai bot commented Sep 3, 2021

sonarcloud bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

🩹 False positive model validation fail when combining multiple default megacomplexes #797

🩹 False positive model validation fail when combining multiple default megacomplexes #797

Conversation

s-weigand commented Aug 29, 2021 • edited Loading

Change summary

Checklist

Closes issues

github-actions bot commented Aug 29, 2021

codecov bot commented Aug 29, 2021 • edited Loading

Codecov Report

jsnel left a comment

Choose a reason for hiding this comment

joernweissenborn left a comment

Choose a reason for hiding this comment

sourcery-ai bot commented Sep 3, 2021

Sourcery Code Quality Report

Legend and Explanation

sonarcloud bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

s-weigand commented Aug 29, 2021 •

edited

Loading

codecov bot commented Aug 29, 2021 •

edited

Loading