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

[staging] 🐛 Parameters expressions are not evaluated in the correct order #1499

Closed jsnel closed 1 month ago

jsnel commented 2 months ago

Version information

Describe the bug

We observed there were differences in the optimization between main (0.7) and staging (0.8-dev) and found the problem to be related to models in which parameters relations (expression) were used.

After some debugging we found that the order of execution (evaluating parameters) was incorrect, the parameter that had dependencies would itself be added before its dependencies causing it to be evaluated with values from a previous iteration.

To Reproduce

Steps to reproduce the behavior:

Run any model that has parameters relations in it, for instance: study_transient_absorption/transient_absorption_target_analysis.ipynb on main: [transient_absorption_target_analysis.ipynb](https://github.com/glotaran/pyglotaran-examples/blob/main/pyglotaran_examples/study_transient_absorption/transient_absorption_target_analysis.ipynb) on staging: transient_absorption_target_analysis

Then observe that the optimization (given sufficient number of iterations) is following a different trajectory. If no update to the relation parameter happens (e.g. it is fixed, or there are not sufficient iterations/function evaluations possible) you will not see it. Also the first iteration is always idential because the initial parameter values are used.

Important note is that on main the order in which parameters are evlauated is the order in which they were put in the parameters specification file, thus to ensure correctness, relation parameters need to be before the parameters in which they are used in an expresssion.

Expected behavior

Both main and staging follow the same optimization trajectory and arrive at the same final result.

Screenshots

Additional context

jsnel commented 1 month ago

This was fixed on staging.

Should also be considered for backporting to v0.7.x branch, but that can be a seperate issue.