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

🚀🩹 v0.4.2 release preparation and bugfix backport for result consistency checking purposes #935

Merged

Conversation

jsnel
Copy link
Member

@jsnel jsnel commented Dec 11, 2021

Maintenance release v0.4.2 with bugfix backport for result consistency checking purposes

🩹🚧 Backport of bugfix #927 discovered in PR #860 related to initial_concentration normalization when saving results (#935).

It was found in #860 that the a_matrix when calculated for result saving was calculated without normalized initial_concentrations while the calculation for the optimization was (correctly) using the normalized version. Fixing this bug in #927 then breaks result consistency checking for some case studies, thus requiring backporting of this bugfix to the 0.4 maintenance branch which we continue to use for result/consistency checking.

Checklist

  • ✔️ Passing the tests (mandatory for all PR's)
  • 👌 Link to issue 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

Backport of bugfix glotaran#927 discovered in PR glotaran#860

It was [found](glotaran#860 (comment)) in glotaran#860 that the a_matrix when calculated for result saving was calculated without normalized initial_concentrations while the calculation for the optimization was (correctly) using the normalized version. Fixing this bug in glotaran#927 then breaks result consistency checking for some case studies, thus requiring backporting of this bugfix to the 0.4 maintenance branch which we continue to use for result/consistency checking.
@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch jsnel/pyglotaran/maintenance/v0.4-bugfix-a_matrix

- 🩹🚧 Backport of bugfix glotaran#927 discovered in PR glotaran#860 related to initial_concentration normalization when saving results (glotaran#935).
@jsnel jsnel force-pushed the maintenance/v0.4-bugfix-a_matrix branch from 8677c65 to a83bcb3 Compare December 11, 2021 23:30
@codecov
Copy link

codecov bot commented Dec 11, 2021

Codecov Report

Merging #935 (8f5f91a) into maintenance/v0.4.1 (21ba272) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           maintenance/v0.4.1    #935   +/-   ##
==================================================
  Coverage                81.8%   81.8%           
==================================================
  Files                      79      79           
  Lines                    3924    3924           
  Branches                  692     719   +27     
==================================================
  Hits                     3212    3212           
  Misses                    593     593           
  Partials                  119     119           
Impacted Files Coverage Δ
glotaran/__init__.py 100.0% <100.0%> (ø)
...iltin/models/kinetic_image/kinetic_image_result.py 85.7% <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 21ba272...8f5f91a. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2021

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

Benchmark diff
· Did not find results for commit 2c2c6053920778ae7aff075f11333152af5d82fb

@jsnel
Copy link
Member Author

jsnel commented Dec 11, 2021

The first test run of this PR ( a83bcb3) is expected to fail on the comparison results consistency check because a bugfix was committed which changes how the results are saved (the a_matrix and derived DAS in the results was previously saved using the normalized initial_concentrations, this was a bug fixed in this PR). After confirming this, the 'gold standard' can be updated to reflect these changes.

[update] Results confirming failing tests:

Updated pyglotaran-examples result-comparison branch for pyglotaran v0.4.2
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 12, 2021

Sourcery Code Quality Report

Merging this PR leaves code quality unchanged.

Quality metrics Before After Change
Complexity 0.67 ⭐ 0.67 ⭐ 0.00
Method Length 24.50 ⭐ 24.50 ⭐ 0.00
Working memory 4.33 ⭐ 4.33 ⭐ 0.00
Quality 91.44% 91.44% 0.00%
Other metrics Before After Change
Lines 22 22 0
Changed files Quality Before Quality After Quality Change
glotaran/init.py 91.44% ⭐ 91.44% ⭐ 0.00%

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

File Function Complexity Length Working Memory Quality Recommendation

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
Copy link

sonarcloud bot commented Dec 12, 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@s-weigand s-weigand added Type: Bug Minor issues, non-crashing bug, slowdowns Type: Release preparation Preparations for a new release (e.g. bumping version and updating history) labels Dec 12, 2021
@jsnel
Copy link
Member Author

jsnel commented Dec 12, 2021

After the update we find that the result comparison still fails but only for a singular data point in one of the examples (log below). Digging a little deeper (looking at the plot) we find that the point of failure is at a place were the spectral value in question is very small (~-1.497), compared to the overall maximum of the spectrum (~7500). However, the spectra are compared locally (per wavelength) and the absolute tolerance for the comparison between results is scaled with the local value of the data, meaning that the tolerance for a point close to zero is much tighter than the tolerance for a point near the maximum. This is a point where the result comparison could be further refined, but for now we accept this as a PASS ✔️ and this PR may be merged.

FAILED .github/test_result_consistency.py::test_result_data_var_consistency[study_fluorescence-decay_associated_spectra]
E       AssertionError: Result data_var data mismatch: 'decay_associated_spectra' in 'dataset1.nc'.
E         With sum of absolute difference: 0.0039020775839004873 and shape: (49, 5)
E         Mean difference: 1.592684728122648e-05
E         
E       assert False
E        +  where False = <function allclose.<locals>._allclose at 0x7f40599e5790>(<xarray.DataArray 'decay_associated_spectra' (spectral: 49, component: 5)>\narray([[-145.581345,   52.590453,   21.6330...7\n    rate      (component) float64 ...\n    lifetime  (component) float64 ...\nDimensions without coordinates: component, <xarray.DataArray 'decay_associated_spectra' (spectral: 49, component: 5)>\narray([[-145.581346,   52.590453,   21.6330...7\n    rate      (component) float64 ...\n    lifetime  (component) float64 ...\nDimensions without coordinates: component, atol=array([[1.73546487e-05, 6.26927054e-06, 2.57886253e-06, 2.17881660e-06,\n        6.32769907e-06],\n       [1.50005268e-0...     5.91504505e-06],\n       [5.92400043e-05, 4.45607905e-05, 4.22466039e-06, 1.01596965e-04,\n        6.42848991e-06]]), rtol=1e-05, print_fail=20)

.github/test_result_consistency.py:221: AssertionError
----------------------------- Captured stdout call -----------------------------
allclose first 1 failures:
  (40, 1): -1.4969902223364386 -1.4969635695838823
=========================== short test summary info ============================
FAILED .github/test_result_consistency.py::test_result_data_var_consistency[study_fluorescence-decay_associated_spectra]

@jsnel jsnel merged commit d2bff46 into glotaran:maintenance/v0.4.1 Dec 12, 2021
jsnel pushed a commit that referenced this pull request Dec 12, 2021
…r decay megacomplex (#927)

Bugfix also back-ported to v0.4 maintenance branch in PR #935
@s-weigand s-weigand deleted the maintenance/v0.4-bugfix-a_matrix branch December 12, 2021 15:03
@jsnel jsnel mentioned this pull request Dec 31, 2021
1 task
jsnel added a commit that referenced this pull request Dec 31, 2021
- 🩹🚧 Backport of bugfix 🩹 Bugfix Use normalized initial_concentrations in result creation for decay megacomplex #927 
- 📚 Updated Readme (PyPI badge link, Discord URL, DOI)
- 📚 Updated Changelog to reflect changes
- Follow up on: 🚀🩹 v0.4.2 release preparation and bugfix backport for result consistency checking purposes #935).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Minor issues, non-crashing bug, slowdowns Type: Release preparation Preparations for a new release (e.g. bumping version and updating history)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants