Refactored ModelProperty. #914

joernweissenborn · 2021-11-26T15:41:23Z

This PR refactors the ModelProperty class.

Change summary

Markdown printing of Model has been changed
- Beautification is left for followup PR

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)
📚 Adds documentation of the feature

Closes issues

closes #901

github-actions · 2021-11-26T15:41:38Z

👈 Launch a binder notebook on branch joernweissenborn/pyglotaran/refactor/propertymarkdown

github-actions · 2021-11-26T15:44:59Z

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

Benchmark diff v0.5.0rc1 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [d05c042a]       [210bdb80]
     <v0.5.0rc1>                 
         72.0±2ms         76.1±2ms     1.06  BenchmarkOptimize.time_optimize(False, False, False)
         175±10ms         167±20ms     0.95  BenchmarkOptimize.time_optimize(False, False, True)
         73.1±1ms         76.6±2ms     1.05  BenchmarkOptimize.time_optimize(False, True, False)
         144±30ms          156±7ms     1.08  BenchmarkOptimize.time_optimize(False, True, True)
         95.0±3ms         94.9±3ms     1.00  BenchmarkOptimize.time_optimize(True, False, False)
         108±30ms          104±3ms     0.96  BenchmarkOptimize.time_optimize(True, False, True)
         92.0±1ms         96.6±1ms     1.05  BenchmarkOptimize.time_optimize(True, True, False)
         140±40ms         107±40ms    ~0.76  BenchmarkOptimize.time_optimize(True, True, True)
             197M             199M     1.01  IntegrationTwoDatasets.peakmem_optimize
+      1.91±0.04s       2.46±0.05s     1.29  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)


All benchmarks:

       before           after         ratio
     [979643ac]       [210bdb80]
         80.9±3ms         76.1±2ms     0.94  BenchmarkOptimize.time_optimize(False, False, False)
         150±40ms         167±20ms    ~1.11  BenchmarkOptimize.time_optimize(False, False, True)
         76.1±2ms         76.6±2ms     1.01  BenchmarkOptimize.time_optimize(False, True, False)
         165±20ms          156±7ms     0.95  BenchmarkOptimize.time_optimize(False, True, True)
         95.7±2ms         94.9±3ms     0.99  BenchmarkOptimize.time_optimize(True, False, False)
         106±40ms          104±3ms     0.98  BenchmarkOptimize.time_optimize(True, False, True)
         94.6±2ms         96.6±1ms     1.02  BenchmarkOptimize.time_optimize(True, True, False)
         112±30ms         107±40ms     0.95  BenchmarkOptimize.time_optimize(True, True, True)
             196M             199M     1.01  IntegrationTwoDatasets.peakmem_optimize
       2.28±0.07s       2.46±0.05s     1.08  IntegrationTwoDatasets.time_optimize

codecov · 2021-11-26T15:45:44Z

Codecov Report

Merging #914 (210bdb8) into main (979643a) will increase coverage by 0.5%.
The diff coverage is 91.9%.

@@           Coverage Diff           @@
##            main    #914     +/-   ##
=======================================
+ Coverage   84.5%   85.1%   +0.5%     
=======================================
  Files         85      85             
  Lines       4785    4793      +8     
  Branches     891     920     +29     
=======================================
+ Hits        4047    4079     +32     
+ Misses       589     562     -27     
- Partials     149     152      +3

Impacted Files	Coverage Δ
glotaran/parameter/parameter.py	`95.4% <63.6%> (+4.1%)`	⬆️
glotaran/model/property.py	`92.5% <91.8%> (+6.9%)`	⬆️
glotaran/model/item.py	`95.4% <94.7%> (+1.9%)`	⬆️
glotaran/model/megacomplex.py	`100.0% <100.0%> (+1.4%)`	⬆️
glotaran/model/model.py	`85.5% <100.0%> (ø)`
glotaran/model/util.py	`85.2% <100.0%> (+14.7%)`	⬆️

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 979643a...210bdb8. Read the comment docs.

glotaran/model/test/test_model_property.py

glotaran/model/property.py

glotaran/parameter/parameter.py

Co-authored-by: Sebastian Weigand <[email protected]>

s-weigand

Functionality wise this looks good to me 👍
So thanks a lot, users will love the enhanced information they get. ❤️

But also it works quite well for 0.6.0 we should really reflect on if we really need this kind of metaprogramming or if we could maybe solve our requirements in a more maintainable fashion and pythonic way, instead of our mad scientist (e.g. utilizing attrs).

For me the most problematic point is inheriting a from the property decorator class, which leads this strange tests in test_model_property that don't reflect the actual usage.
The more tests the married, but they should reflect the usage in the code.

For now, there is no quick way around it (at least after 2h of trial and error from my side) and it would be out of scope for this PR, since this is an integral part of the code for 3 years.

sourcery-ai · 2021-11-28T23:21:13Z

Sourcery Code Quality Report

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

Quality metrics	Before	After	Change
Complexity	7.76 ⭐	5.11 ⭐	-2.65 👍
Method Length	50.75 ⭐	47.74 ⭐	-3.01 👍
Working memory	7.06 🙂	6.97 🙂	-0.09 👍
Quality	69.62% 🙂	72.74% 🙂	3.12% 👍

Other metrics	Before	After	Change
Lines	2794	3029	235

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/io/yml/test/test_model_parser.py	82.35% ⭐	80.11% ⭐	-2.24% 👎
glotaran/model/item.py	56.94% 🙂	64.34% 🙂	7.40% 👍
glotaran/model/megacomplex.py	72.53% 🙂	74.48% 🙂	1.95% 👍
glotaran/model/model.py	70.83% 🙂	70.83% 🙂	0.00%
glotaran/model/property.py	48.70% 😞	58.53% 🙂	9.83% 👍
glotaran/model/util.py	83.31% ⭐	86.62% ⭐	3.31% 👍
glotaran/model/test/test_model.py	72.58% 🙂	72.61% 🙂	0.03% 👍
glotaran/parameter/parameter.py	85.18% ⭐	84.83% ⭐	-0.35% 👎

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

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
glotaran/model/property.py	ModelProperty.glotaran_validate	33 ⛔	167 😞	12 😞	32.34% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/model/item.py	model_item	18 🙂	292 ⛔	8 🙂	41.32% 😞	Try splitting into smaller methods
glotaran/model/property.py	ModelProperty.glotaran_fill	20 😞	133 😞	13 😞	41.71% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/model/property.py	_model_property_setter_factory	14 🙂	139 😞	14 😞	44.28% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/model/property.py	_model_property_setter_factory.setter	11 🙂	132 😞	14 😞	47.70% 😞	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!

sonarqubecloud · 2021-11-28T23:21:32Z

SonarCloud Quality Gate failed.

14 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

jsnel

LGTM, but I do agree and want to place emphasis on @s-weigand 's findings:

For me the most problematic point is inheriting a from the property decorator class, which leads this strange tests in test_model_property that don't reflect the actual usage.
The more tests the married, but they should reflect the usage in the code.

As (now) picked up by SonarCloud:
https://sonarcloud.io/summary/new_code?id=glotaran_pyglotaran&pullRequest=914

Let's add it to the v0.6.0 list.

Refactored ModelProperty.

97be7f8

joernweissenborn requested a review from jsnel as a code owner November 26, 2021 15:41

joernweissenborn requested a review from s-weigand November 26, 2021 15:44

joernweissenborn commented Nov 26, 2021

View reviewed changes

glotaran/model/test/test_model_property.py Outdated Show resolved Hide resolved

Update glotaran/model/test/test_model_property.py

436861a

joernweissenborn commented Nov 26, 2021

View reviewed changes

glotaran/model/property.py Outdated Show resolved Hide resolved

joernweissenborn commented Nov 26, 2021

View reviewed changes

glotaran/model/property.py Outdated Show resolved Hide resolved

joernweissenborn added 8 commits November 27, 2021 12:20

Fixed rturn type.

6a85f9a

Removed dead code.

1ba4941

Renamed _create_XXX functions to _XXX_factory.

300dbd1

Replaced exception with value error..

38e58df

Cleaner type inspection

202a569

Removed typing inspect dependency

ba425c8

added mypycheck to model/property

1de5af9

Added doc to model/property.py

d9ef1a5

joernweissenborn requested a review from a team as a code owner November 27, 2021 12:36

s-weigand reviewed Nov 27, 2021

View reviewed changes

glotaran/parameter/parameter.py Outdated Show resolved Hide resolved

joernweissenborn and others added 3 commits November 28, 2021 13:15

Update glotaran/parameter/parameter.py

281ea03

Co-authored-by: Sebastian Weigand <[email protected]>

📚🩹 Fix missing f in front of f-string

7cf1f7a

🧹 Removed obsolete dependency on typing-inspect from requirements_dev

e91d061

s-weigand mentioned this pull request Nov 28, 2021

🐛 Improve ParameterGroup markdown rendering #916

Closed

🧹🧪 Improved readability of property markdown test

210bdb8

s-weigand previously approved these changes Nov 28, 2021

View reviewed changes

s-weigand dismissed their stale review via 210bdb8 November 28, 2021 23:20

s-weigand approved these changes Nov 28, 2021

View reviewed changes

jsnel approved these changes Nov 29, 2021

View reviewed changes

jsnel mentioned this pull request Nov 29, 2021

Further refactor ModelProperty #917

Closed

jsnel merged commit 88111bb into glotaran:main Nov 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored ModelProperty. #914

Refactored ModelProperty. #914

joernweissenborn commented Nov 26, 2021 •

edited

Loading

github-actions bot commented Nov 26, 2021

github-actions bot commented Nov 26, 2021 •

edited

Loading

codecov bot commented Nov 26, 2021 •

edited

Loading

s-weigand left a comment

sourcery-ai bot commented Nov 28, 2021

sonarqubecloud bot commented Nov 28, 2021

jsnel left a comment •

edited

Loading

Refactored ModelProperty. #914

Refactored ModelProperty. #914

Conversation

joernweissenborn commented Nov 26, 2021 • edited Loading

Change summary

Checklist

Closes issues

github-actions bot commented Nov 26, 2021

github-actions bot commented Nov 26, 2021 • edited Loading

codecov bot commented Nov 26, 2021 • edited Loading

Codecov Report

s-weigand left a comment

Choose a reason for hiding this comment

sourcery-ai bot commented Nov 28, 2021

Sourcery Code Quality Report

Legend and Explanation

sonarqubecloud bot commented Nov 28, 2021

jsnel left a comment • edited Loading

Choose a reason for hiding this comment

joernweissenborn commented Nov 26, 2021 •

edited

Loading

github-actions bot commented Nov 26, 2021 •

edited

Loading

codecov bot commented Nov 26, 2021 •

edited

Loading

jsnel left a comment •

edited

Loading