glotaran / pyglotaran

A Python library for Global and Target Analysis of time-resolved spectroscopy data
GNU Lesser General Public License v3.0
53 stars 18 forks source link

🩹Fix order of parameters involved in parameters expresssion #1500

Closed jsnel closed 1 month ago

jsnel commented 2 months ago

Add the dependent parameters after its dependencies.

Parameters involved in a relation (parameter expression) should be added before the parameter that declares a dependency on them.

Change summary

Checklist

Closes issues

closes #1499

github-actions[bot] commented 2 months ago

Binder :point_left: Launch a binder notebook on branch _jsnel/pyglotaran/fix/staging_fix_wrong_order_parameter_expressioneval

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.3%. Comparing base (8dfac94) to head (fe69696).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## staging #1500 +/- ## ======================================= Coverage 85.2% 85.3% ======================================= Files 91 91 Lines 3756 3756 Branches 734 734 ======================================= + Hits 3203 3205 +2 + Misses 440 439 -1 + Partials 113 112 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jsnel commented 2 months ago

Status of the compare results test

Currently on staging: ================ 159 failed, 418 passed, 49 warnings in 14.00s =================

With the changes in this PR: ================ 148 failed, 429 passed, 49 warnings in 13.75s =================

jsnel commented 2 months ago

TODO: add some unit tests for parameter resolving (this is why PR is still in DRAFT)

jsnel commented 2 months ago

Reduced the number of errors in the comparison further by adjusting the order of the scheme definition in the fluorescence example to preserve the order of compartments.

New status: ================ 141 failed, 436 passed, 49 warnings in 14.14s =================

joernweissenborn commented 1 month ago

This is actually lingering much longer, like very long. I never thought much about it but yeah, parameter order matters with related stuff.

The fix is totally correct. Would lgtm it now but you want to add a test.

sourcery-ai[bot] commented 1 month ago

🧙 Sourcery has finished reviewing your pull request!


Tips - Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
jsnel commented 1 month ago

Unit tests were added to check (specifically) for the issue that this PR fixes (incorrect order of parameters).

The final result remains unchanged: ================ 141 failed, 436 passed, 49 warnings in 13.83s =================

The PR is ready for review now.