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.

Sign up for GitHub

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

Jump to bottom

🩹 Fix and re-enable IRF Dispersion Test #786

Merged
merged 2 commits into from
Aug 26, 2021
Merged

🩹 Fix and re-enable IRF Dispersion Test #786

merged 2 commits into from
Aug 26, 2021

Conversation

jsnel
Copy link
Member

@jsnel jsnel commented Aug 22, 2021 •
edited
Loading

Improve the tests in test_spectral_irf
Added NoDispersion test
Re-enable IrfDispersion tests on the CI (de-Volkswagen the tests)
Improve assignment of center_dispersion to result dataset

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

@jsnel jsnel requested a review from s-weigand August 22, 2021 21:06
@jsnel jsnel requested review from joernweissenborn and a team as code owners August 22, 2021 21:06
@github-actions
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Aug 22, 2021 •
edited
Loading

Codecov Report

Merging #786 (8bc9c31) into staging (a6a5a51) will increase coverage by 0.4%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           staging    #786     +/-   ##
=========================================
+ Coverage     83.9%   84.3%   +0.4%     
=========================================
  Files           75      75             
  Lines         4186    4191      +5     
  Branches       754     756      +2     
=========================================
+ Hits          3513    3537     +24     
+ Misses         538     520     -18     
+ Partials       135     134      -1
Impacted Files Coverage Δ
glotaran/builtin/megacomplexes/decay/irf.py 86.4% <100.0%> (+20.2%) ⬆️
glotaran/builtin/megacomplexes/decay/util.py 73.1% <100.0%> (+2.7%) ⬆️
glotaran/deprecation/modules/builtin_io_yml.py 100.0% <100.0%> (ø)
glotaran/builtin/megacomplexes/spectral/shape.py 81.8% <0.0%> (+3.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 a6a5a51...8bc9c31. Read the comment docs.

@jsnel jsnel force-pushed the refactor/fix_test_spectral_irf branch from 02cc5a8 to 59f82c0 Compare August 22, 2021 21:19
@jsnel
Fix and re-enable IRF Dispersion Test
73d16d8 
Improve the tests in test_spectral_irf
Added NoDispersion test
Re-enable IrfDispersion tests on the CI (de-Volkswagen the tests)
Improve assignment of center_dispersion to result dataset
@jsnel jsnel force-pushed the refactor/fix_test_spectral_irf branch from 59f82c0 to 73d16d8 Compare August 26, 2021 21:38
@jsnel
Rename center_dispersion to irf_center_location in result
8bc9c31 
The name irf_center_location in the results describes more clearly what the variable holds.
Deprecated irf model item center_dispersion  for center_dispersion_coefficients
Deprecated irf model item width_dispersion  for width_dispersion_coefficients
@sonarcloud
Copy link

sonarcloud bot commented Aug 26, 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 8 Code Smells

No Coverage information No Coverage information
0.0% 0.0% 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]       [8bc9c314]
     <v0.4.0>                   
-        59.6±2ms       41.0±0.5ms     0.69  BenchmarkOptimize.time_optimize(False, False, False)
-        348±10ms         52.4±2ms     0.15  BenchmarkOptimize.time_optimize(False, False, True)
-        81.6±1ms         70.6±3ms     0.86  BenchmarkOptimize.time_optimize(False, True, False)
         83.2±3ms         78.4±4ms     0.94  BenchmarkOptimize.time_optimize(False, True, True)
         62.6±1ms         57.5±1ms     0.92  BenchmarkOptimize.time_optimize(True, False, False)
-         339±6ms        67.2±50ms     0.20  BenchmarkOptimize.time_optimize(True, False, True)
         81.4±1ms         85.9±2ms     1.05  BenchmarkOptimize.time_optimize(True, True, False)
         84.2±2ms         101±50ms    ~1.19  BenchmarkOptimize.time_optimize(True, True, True)
             180M             179M     0.99  IntegrationTwoDatasets.peakmem_create_result
             196M             194M     0.99  IntegrationTwoDatasets.peakmem_optimize
-         254±6ms          202±5ms     0.79  IntegrationTwoDatasets.time_create_result
-      5.42±0.04s       1.44±0.06s     0.27  IntegrationTwoDatasets.time_optimize

@jsnel jsnel requested a review from s-weigand August 26, 2021 23:33
s-weigand
s-weigand approved these changes Aug 26, 2021
View reviewed changes
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.

LGTM

@jsnel jsnel merged commit b281799 into glotaran:staging Aug 26, 2021
@jsnel jsnel deleted the refactor/fix_test_spectral_irf branch August 26, 2021 23:35
jsnel added a commit that referenced this pull request Aug 29, 2021
@jsnel
Cleanup IRF Dispersion test PR #786
9dc64af 
Address a trivial Sourcery concern to avoid further PRs
Reset low relative tolerance back to default when comparing parameter values
Added some helpful hints to comparison asserts in case of failure
This was referenced Sep 8, 2021
Closed
Closed
@jsnel jsnel mentioned this pull request Sep 16, 2021
Merged
5 tasks
jsnel added a commit that referenced this pull request Sep 16, 2021
@jsnel
🩹 Fix and re-enable IRF Dispersion Test (#786)
77d3950 
* Fix and re-enable IRF Dispersion Test

Improve the tests in test_spectral_irf
Added NoDispersion test
Re-enable IrfDispersion tests on the CI (de-Volkswagen the tests)
Improve assignment of center_dispersion to result dataset

* Rename center_dispersion to irf_center_location in result

The name irf_center_location in the results describes more clearly what the variable holds.
Deprecated irf model item center_dispersion  for center_dispersion_coefficients
Deprecated irf model item width_dispersion  for width_dispersion_coefficients
jsnel added a commit that referenced this pull request Sep 16, 2021
@jsnel
Cleanup IRF Dispersion test PR #786
5ea9385 
Address a trivial Sourcery concern to avoid further PRs
Reset low relative tolerance back to default when comparing parameter values
Added some helpful hints to comparison asserts in case of failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-weigand s-weigand s-weigand approved these changes

@joernweissenborn joernweissenborn Awaiting requested review from joernweissenborn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jsnel @s-weigand