Refactor/spectral model #763

joernweissenborn · 2021-08-08T11:52:56Z

This PR introduces some changes to the spectral model in order to "straighten it out" and enable TIMP like behaviour.

Change summary

Introduced SpectralShapeGaussian as shape type gaussian
Made skewness property mandatory for shape skewed-gaussian
removed energy_spectrum property from spectral megacomplex
added spectral_axis_inverted and spectral_axis_scale to dataset model (see below)

Changes to spectral axis

The old behaviour was to invert the axis as axis = 1e7 / axis if energy_spectrum was set true in the corresponding megacomplex.

The new behaviour inverts the axis as axis = dataset_model.spectral_axis_scale / axis if spectral_axis_inverted property of the dataset model is set true. The spectral_axis_scale is also a dataset model property and is default 1. If the inverted property is not set and scale != 1 is provided, the axis will be scaled as axis = dataset_model.spectral_axis_scale * axis

Checklist

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

…and axis_scale.

github-actions · 2021-08-08T11:53:08Z

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

codecov · 2021-08-08T11:56:35Z

Codecov Report

Merging #763 (6bb9ae5) into staging (f41949f) will increase coverage by 0.4%.
The diff coverage is 80.0%.

@@            Coverage Diff            @@
##           staging    #763     +/-   ##
=========================================
+ Coverage     80.4%   80.8%   +0.4%     
=========================================
  Files           70      71      +1     
  Lines         4043    4061     +18     
  Branches       720     730     +10     
=========================================
+ Hits          3252    3285     +33     
+ Misses         664     645     -19     
- Partials       127     131      +4

Impacted Files	Coverage Δ
glotaran/examples/sequential.py	`100.0% <ø> (+100.0%)`	⬆️
...tin/megacomplexes/spectral/spectral_megacomplex.py	`84.4% <50.0%> (+0.7%)`	⬆️
glotaran/builtin/megacomplexes/spectral/shape.py	`78.7% <66.6%> (-13.6%)`	⬇️
glotaran/utils/sanitize.py	`46.4% <74.2%> (ø)`
glotaran/builtin/io/yml/yml.py	`80.4% <100.0%> (ø)`
glotaran/model/item.py	`95.8% <100.0%> (+2.8%)`	⬆️
glotaran/model/property.py	`93.4% <100.0%> (+0.2%)`	⬆️
glotaran/parameter/parameter.py	`98.7% <100.0%> (-0.1%)`	⬇️
glotaran/utils/regex.py	`100.0% <100.0%> (ø)`
... and 2 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 f41949f...6bb9ae5. Read the comment docs.

…/pyglotaran into refactor/spectral_model

glotaran/builtin/megacomplexes/spectral/shape.py

added fallback for skewness == 0.

s-weigand

Besides the requested change to the docstring, this LGTM.

glotaran/builtin/megacomplexes/spectral/shape.py

s-weigand · 2021-08-08T14:24:17Z

Also, one more minor side note, don't use blank # noqa, use # noqa: T001 instead.
Or even better add a per file ignore to the flake8 config (tox.ini L22), that way you don't need to use it over and over again.

    # Allow printing in test files
    test_*.py: T001

joernweissenborn · 2021-08-08T16:46:26Z

Also, one more minor side note, don't use blank # noqa, use # noqa: T001 instead.
Or even better add a per file ignore to the flake8 config (tox.ini L22), that way you don't need to use it over and over again.
    # Allow printing in test files
    test_*.py: T001

We should do this in a followup PR where we add flake8-print to pre-commit.

s-weigand

Should be fine now

Update typing and docstrings Correct typo sanatize -> sanitize (incl rename to sanitize.py)

Move sanatize.py to sanitize.py and update dependencies

Convert scientific notation string (e.g. 1E7) to proper floats

…d axis

…loat or int typed properties which are parsed as strings.

Refactor/spectral model addendum

sonarqubecloud · 2021-08-12T14:49: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-08-12T14:49:58Z

Sourcery Code Quality Report

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

Quality metrics	Before	After	Change
Complexity	8.19 ⭐	7.71 ⭐	-0.48 👍
Method Length	41.63 ⭐	38.94 ⭐	-2.69 👍
Working memory	8.19 🙂	8.44 🙂	0.25 👎
Quality	68.44% 🙂	68.82% 🙂	0.38% 👍

Other metrics	Before	After	Change
Lines	1206	927	-279

Changed files	Quality Before	Quality After	Quality Change
glotaran/builtin/io/yml/yml.py	52.46% 🙂	52.46% 🙂	0.00%
glotaran/examples/sequential.py	46.88% 😞	46.88% 😞	0.00%
glotaran/model/property.py	42.70% 😞	43.98% 😞	1.28% 👍
glotaran/parameter/parameter.py	88.66% ⭐	88.47% ⭐	-0.19% 👎

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.validate	44 ⛔	187 😞	13 😞	25.34% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/builtin/io/yml/yml.py	YmlProjectIo.load_scheme	13 🙂	257 ⛔	15 😞	34.68% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/model/property.py	ModelProperty.__init__	21 😞	196 😞	13 😞	34.91% 😞	Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/model/property.py	ModelProperty.__init__.setter	14 🙂	121 😞	13 😞	47.83% 😞	Try splitting into smaller methods. Extract out complex expressions
glotaran/model/property.py	ModelProperty.fill	17 🙂	124 😞	11 😞	48.33% 😞	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-08-12T15:41:22Z

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]       [6bb9ae55]
     <v0.4.0>                   
-      42.6±0.1ms       32.0±0.1ms     0.75  BenchmarkOptimize.time_optimize(False, False, False)
-       234±0.3ms        38.0±10ms     0.16  BenchmarkOptimize.time_optimize(False, False, True)
-      62.1±0.3ms       52.9±0.3ms     0.85  BenchmarkOptimize.time_optimize(False, True, False)
       64.1±0.4ms         61.4±9ms     0.96  BenchmarkOptimize.time_optimize(False, True, True)
       43.3±0.4ms       41.5±0.6ms     0.96  BenchmarkOptimize.time_optimize(True, False, False)
-       234±0.8ms       46.7±0.7ms     0.20  BenchmarkOptimize.time_optimize(True, False, True)
       62.6±0.2ms       63.3±0.5ms     1.01  BenchmarkOptimize.time_optimize(True, True, False)
+      64.7±0.2ms         72.4±4ms     1.12  BenchmarkOptimize.time_optimize(True, True, True)
             179M             181M     1.01  IntegrationTwoDatasets.peakmem_create_result
             194M             198M     1.02  IntegrationTwoDatasets.peakmem_optimize
-         200±1ms          160±2ms     0.80  IntegrationTwoDatasets.time_create_result
-      4.14±0.01s       1.20±0.03s     0.29  IntegrationTwoDatasets.time_optimize

jsnel

Looks good to me now.

* Added SpectralShapeGaussian. * Replaced property energy_spectrum of SpectralMegacomplex with invert and axis_scale. * Made invert and axis_scale dataset properties. * Added guard and meaningful exception message for spectral skewness * Fixed scaling * Made SpectralShapeSkewedGaussian decendent of SpectralShapeGaussian and added fallback for skewness == 0. * Move sanatize.py to utils module and correct typo sanatize -> sanitize (incl rename to sanitize.py) * Move regex patterns to seperate module in utils * Add sanity_scientific_notation_conversion Convert scientific notation string (e.g. 1E7) to proper floats * Fixed spectral megacomplex test parameters and added test for inverted axis * Removed model_item._from_list * Added convenience in model_item.from_dict for automatically convert float or int typed properties which are parsed as strings. * Made amplitude of shape optional Co-authored-by: Joris Snellenburg <[email protected]>

joernweissenborn added 3 commits August 8, 2021 13:29

Added SpectralShapeGaussian.

1e9c845

Replaced property energy_spectrum of SpectralMegacomplex with invert …

a78bc3a

…and axis_scale.

Made invert and axis_scale dataset properties.

60f9381

joernweissenborn requested a review from jsnel as a code owner August 8, 2021 11:52

joernweissenborn added 2 commits August 8, 2021 13:57

Made invert and axis_scale dataset properties.

809d257

Merge branch 'refactor/spectral_model' of github.com:joernweissenborn…

b610dfa

…/pyglotaran into refactor/spectral_model

joernweissenborn requested a review from a team as a code owner August 8, 2021 11:58

s-weigand reviewed Aug 8, 2021

View reviewed changes

glotaran/builtin/megacomplexes/spectral/shape.py Show resolved Hide resolved

joernweissenborn added 3 commits August 8, 2021 14:33

Added guard and meaningfull exception message for spectral skewness

98b6b66

Fixed scaling

40ad668

Made SpectralShapeSkewedGaussian decendent of SpectralShapeGaussian and

8aba24f

added fallback for skewness == 0.

s-weigand requested changes Aug 8, 2021

View reviewed changes

glotaran/builtin/megacomplexes/spectral/shape.py Show resolved Hide resolved

Fixed docstring

ac12fcd

s-weigand approved these changes Aug 8, 2021

View reviewed changes

jsnel and others added 8 commits August 12, 2021 02:24

Move sanatize.py to utils module

ac99115

Update typing and docstrings Correct typo sanatize -> sanitize (incl rename to sanitize.py)

Move regex patterns to seperate module in utils

043bb48

Move sanatize.py to sanitize.py and update dependencies

Add sanity_scientific_notation_conversion

e1458fe

Convert scientific notation string (e.g. 1E7) to proper floats

Fixed spectral megacomplex test parameters and added test for inverte…

ff6392c

…d axis

Removed model_item._from_list

a3d1b08

Added convinience in model_item.from_dict for automatically convert f…

78078f0

…loat or int typed properties which are parsed as strings.

Made amplitude of shape optional

f6f1ca4

Merge pull request #3 from jsnel/refactor/spectral_model_addendum

6bb9ae5

Refactor/spectral model addendum

jsnel approved these changes Aug 12, 2021

View reviewed changes

jsnel merged commit cee1834 into glotaran:staging Aug 12, 2021

s-weigand mentioned this pull request Aug 12, 2021

🔧 Use flake8-print in pre-commit #772

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/spectral model #763

Refactor/spectral model #763

joernweissenborn commented Aug 8, 2021 •

edited

Loading

github-actions bot commented Aug 8, 2021

codecov bot commented Aug 8, 2021 •

edited

Loading

s-weigand left a comment

s-weigand commented Aug 8, 2021

joernweissenborn commented Aug 8, 2021

s-weigand left a comment

sonarqubecloud bot commented Aug 12, 2021

sourcery-ai bot commented Aug 12, 2021

github-actions bot commented Aug 12, 2021

jsnel left a comment

Refactor/spectral model #763

Refactor/spectral model #763

Conversation

joernweissenborn commented Aug 8, 2021 • edited Loading

Change summary

Changes to spectral axis

Checklist

github-actions bot commented Aug 8, 2021

codecov bot commented Aug 8, 2021 • edited Loading

Codecov Report

s-weigand left a comment

Choose a reason for hiding this comment

s-weigand commented Aug 8, 2021

joernweissenborn commented Aug 8, 2021

s-weigand left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 12, 2021

sourcery-ai bot commented Aug 12, 2021

Sourcery Code Quality Report

Legend and Explanation

github-actions bot commented Aug 12, 2021

jsnel left a comment

Choose a reason for hiding this comment

joernweissenborn commented Aug 8, 2021 •

edited

Loading

codecov bot commented Aug 8, 2021 •

edited

Loading