Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor model.from_dict to parse megacomplex_type from dict and add simple_generator for testing #807

Merged
merged 24 commits into from
Sep 16, 2021

Conversation

jsnel
Copy link
Member

@jsnel jsnel commented Sep 11, 2021

One of the issues with current tests is that they are hard to read because of large in-line dicts with minimal differences between them, occasionally resulting in the introduction of errors (such as defining a parallel model as a sequential model, illustrated by this commit fixing that error: dce7f0b)

To fix this we need:

  • a simple model + parameters generator
  • Model.from_dict to pickup its type from dict instead of only through passing in kwargs

This PR introduces those changes, and then illustrates the use of the simple generator by using it to refactor a single class (ThreeComponentParallel) used in tests.

This PR opens up the possibility for further refactoring tests for improved readability, to come in a future PR (see #815).

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

Change summary

  • ✨🧪 Added plugin registry monkeypatch context managers for testing
  • ✨🧪 Added new simple_generator for generating model and parameters using a minimal specification (for testing)
  • ♻️ Refactored ThreeComponentParallel in test_decay_megacomplex.py as an example of the new features

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

@jsnel jsnel added the Type: Refactor Refactoring code label Sep 11, 2021
@jsnel jsnel requested a review from s-weigand September 11, 2021 12:14
@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch jsnel/pyglotaran/refactor/megacomplex_types

@codecov
Copy link

codecov bot commented Sep 11, 2021

Codecov Report

Merging #807 (d6a31e6) into staging (0f36985) will increase coverage by 0.2%.
The diff coverage is 91.2%.

Impacted file tree graph

@@            Coverage Diff            @@
##           staging    #807     +/-   ##
=========================================
+ Coverage     84.5%   84.7%   +0.2%     
=========================================
  Files           75      77      +2     
  Lines         4201    4343    +142     
  Branches       757     785     +28     
=========================================
+ Hits          3553    3682    +129     
- Misses         515     521      +6     
- Partials       133     140      +7     
Impacted Files Coverage Δ
glotaran/testing/model_generators.py 88.6% <88.6%> (ø)
glotaran/model/model.py 85.7% <88.8%> (+0.1%) ⬆️
glotaran/builtin/io/yml/yml.py 79.6% <100.0%> (-0.8%) ⬇️
glotaran/testing/plugin_system.py 100.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 0f36985...d6a31e6. Read the comment docs.

jsnel and others added 13 commits September 13, 2021 02:41
The keyword arguments megacomplex_types and default_megacomplex_type are only used for overwrites in testing

Co-authored-by: Sebastian Weigand <[email protected]>
Those context managers allow easy updating or recreating or the plugin registry for test where you don't want to add plugings to the global registry.
Allows the definition of basic models and parameters without all the boilerplate

Note: first draft version, many improvements still possible
Amazing for debugging and printing complex objects
A parallel model only has rates on the diagonal
When rates is set with just a list of floats (without the additional dict element specifying defaults)
model and parameters generated using the simple_generator are shown to be equivalent to the hard coded dicts
@jsnel jsnel force-pushed the refactor/megacomplex_types branch from 92ea07f to c4fd7c4 Compare September 13, 2021 00:42
@jsnel jsnel marked this pull request as ready for review September 13, 2021 00:53
@jsnel jsnel requested review from joernweissenborn and a team as code owners September 13, 2021 00:53
@jsnel jsnel changed the title [WIP] Refactor model.from_dict to use kwargs only for override Refactor model.from_dict to use kwargs only for override Sep 13, 2021
@jsnel jsnel changed the title Refactor model.from_dict to use kwargs only for override Refactor model.from_dict to parse megacomplex_type from dict and add simple_generator for testing Sep 13, 2021
@jsnel
Copy link
Member Author

jsnel commented Sep 13, 2021

This PR opens up the possibility for further refactoring the tests, for instance the other tests in test_decay_megacomplex.py could be refacorted along the same lines:

  • OneComponentOneChannel
  • ThreeComponentSequential
    And perhaps:
  • OneComponentOneChannelGaussianIrf
    (this may requires some further changes to the simple generators)

This could be an easy issue for a new contributor (@s-weigand could you create a new issue for this with some details). This could then be picked up after merging this PR into staging (@joernweissenborn).

jsnel and others added 4 commits September 14, 2021 00:22
Fix in handling irf when not specified
Add basic sanity checking for .rates
Added tests for SimpleGenerator
Added error handling class SimpleGeneratorError

Removed explicit dependency on rich
Clean up mains
@s-weigand
Copy link
Member

Accidentally the pre-commit check for darglint and pydocstyle were "VWed" out for the testing module 😱
I fixed it in 8b4d3bf, sorry the extra noise.

Copy link
Member

@s-weigand s-weigand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor nitpicky things. 😅

Also, could we rename simple_generator.py into something like model_generators.py, if we have the need to create more we can make it a sub_package?
SimpleGenerator sounds like we will need more complex ones.

glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
glotaran/testing/simple_generator.py Outdated Show resolved Hide resolved
jsnel and others added 3 commits September 14, 2021 21:30
Co-authored-by: Sebastian Weigand <[email protected]>
and added docstrings

- Change SimpleGeneratorError to ValueError
- Eliminate SimpleGeneratorError
- Merge _validate_rates and _rates into one
@jsnel jsnel requested a review from s-weigand September 14, 2021 22:35
♻️ Rename simple_generator to model_generators; its purpose is to generate models (together with the parameters specification) based on parameter input values the user assigns to the instance of the model generator

📚 Added darglint compatible docstrings everywhere
@jsnel jsnel force-pushed the refactor/megacomplex_types branch from 862fad3 to c18f791 Compare September 15, 2021 22:46
🩹Use the new monkeypatch_plugin_registry contextmanager in the pytest benchamrks to setup_model
🐛 Make removal of (optional) key fault tolerant
Adding `pylint: disable=W0622` to allow for redefining built-in print
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@github-actions
Copy link
Contributor

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]       [d6a31e66]
     <v0.4.0>                   
-      49.8±0.6ms       36.5±0.4ms     0.73  BenchmarkOptimize.time_optimize(False, False, False)
-       278±0.5ms         43.5±1ms     0.16  BenchmarkOptimize.time_optimize(False, False, True)
-      73.0±0.6ms       61.1±0.4ms     0.84  BenchmarkOptimize.time_optimize(False, True, False)
         75.8±1ms       66.4±0.9ms    ~0.87  BenchmarkOptimize.time_optimize(False, True, True)
       49.7±0.2ms       47.3±0.5ms     0.95  BenchmarkOptimize.time_optimize(True, False, False)
-         278±1ms         56.6±2ms     0.20  BenchmarkOptimize.time_optimize(True, False, True)
       72.9±0.6ms         72.9±1ms     1.00  BenchmarkOptimize.time_optimize(True, True, False)
         77.1±4ms        79.8±50ms     1.04  BenchmarkOptimize.time_optimize(True, True, True)
             180M             180M     1.00  IntegrationTwoDatasets.peakmem_create_result
             198M             197M     1.00  IntegrationTwoDatasets.peakmem_optimize
-         232±3ms          190±3ms     0.82  IntegrationTwoDatasets.time_create_result
       5.02±0.08s       1.84±0.09s    ~0.37  IntegrationTwoDatasets.time_optimize

@jsnel jsnel dismissed s-weigand’s stale review September 16, 2021 00:04

Implemented requested changes: add docstrings to semi-private methods, rename simple_generator to model_generators

@jsnel jsnel merged commit af7addc into glotaran:staging Sep 16, 2021
@jsnel jsnel deleted the refactor/megacomplex_types branch September 16, 2021 00:10
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Sep 16, 2021

Sourcery Code Quality Report

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

Quality metrics Before After Change
Complexity 7.24 ⭐ 7.15 ⭐ -0.09 👍
Method Length 101.71 🙂 96.29 🙂 -5.42 👍
Working memory 11.28 😞 11.42 😞 0.14 👎
Quality 52.46% 🙂 52.74% 🙂 0.28% 👍
Other metrics Before After Change
Lines 206 194 -12
Changed files Quality Before Quality After Quality Change
glotaran/builtin/io/yml/yml.py 52.46% 🙂 52.74% 🙂 0.28% 👍

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

File Function Complexity Length Working Memory Quality Recommendation
glotaran/builtin/io/yml/yml.py YmlProjectIo.load_scheme 13 🙂 257 ⛔ 15 😞 34.68% 😞 Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/io/yml/yml.py YmlProjectIo.save_result 3 ⭐ 214 ⛔ 11 😞 52.52% 🙂 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!

jsnel added a commit that referenced this pull request Sep 16, 2021
…dd simple_generator for testing (#807)

* 👌 Refactor model.from_dict to use kwargs only for override; The keyword arguments megacomplex_types and default_megacomplex_type are only used for overwrites in testing

* ♻️ Refactor a simple DecayModel used in testing

* ✨🧪 Added plugin registry monkeypatch context managers for testing

* 🔧Add rich dependency (Amazing for debugging and printing complex objects)

*  🐛 Fix ThreeComponentParallel to be actually parallel

* 🔧🩹 Fixed  pydocstyle and darglint not picking up the testing module

* 🩹Adapted pytest benchmarks to new monkeypatch_plugin_registry

* 🩹 Ignore Codacy issue W0622 redefining built-in `print` when using `from rich import print`

Co-authored-by: Sebastian Weigand <[email protected]>
Co-authored-by: Sourcery AI <>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants