Visualize: allow for log scale of hierarchical parameters for visualization #1435
Conversation
Doresic
requested review from
stephanmg,
m-philipps,
PaulJonasJost and
dweindl
as code owners
|
|
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1435 +/- ##
===========================================
+ Coverage 83.21% 83.41% +0.19%
===========================================
Files 161 161
Lines 13396 13460 +64
===========================================
+ Hits 11148 11227 +79
+ Misses 2248 2233 -15 ☔ View full report in Codecov by Sentry. |
|
Added the scale description into the labels of parameters into the parameter plot |
pypesto/visualize/parameters.py
Outdated
| x_labels = result.problem.x_names | ||
| # get labels as x_names and scales | ||
| x_labels = [ | ||
| f"{x_name} ({x_scale})" |
There was a problem hiding this comment.
If all parameters use the same scale, the scale could be included once in the x axis label instead of repeatedly in the tick labels.
There was a problem hiding this comment.
Hmmm true, will change
There was a problem hiding this comment.
I changed it, but it is quite a bit of changes to make it work, worth it?
Look at x_axis_label choice after # If all the scales are the same, put it in the x_axis_label comment at line 465, and then it has to be passed back and into parameters_lowlevel. Works tho, and the plot is nicer.
There was a problem hiding this comment.
Works for me, but it was an optional suggestion for now.
and then it has to be passed back and into
parameters_lowlevel.
Alternatively, you could override labels outside parameters_lowlevel, but this doesn't simplify things that much, right?
There was a problem hiding this comment.
Yeah not much, think it would be even uglier if it was outside of there
There was a problem hiding this comment.
Will see what the visualization ppl (@m-philipps and @stephanmg) think.
There was a problem hiding this comment.
Hi, looks fine to me, don't think the repetition of scale label is too bad. Certainly optional to think and implement a more visually pleasing appearance.
| @@ -420,7 +433,7 @@ def noise_inner_parameters_from_parameter_df( | |||
| SplineInnerParameter( | |||
| inner_parameter_id=row[PARAMETER_ID], | |||
| inner_parameter_type=InnerParameterType.SIGMA, | |||
| scale=LIN, | |||
| scale=row[PARAMETER_SCALE], | |||
There was a problem hiding this comment.
If sigma parameters cannot handle the log values, why would we change that here from the LIN?
There was a problem hiding this comment.
True... This is left from a version in which I tried doing it for sigma as well.
But even tho it doesn't work for sigma, it might be good to keep row[PARAMETER_SCALE] here. There are petab checks that will fail before you come to this spot if you have any LOG scale for the sigma parameters. So it feels cleaner to have it taken from petab parameter df instead of hard-coding LIN scale here.
Doresic
changed the title
|
@PaulJonasJost I did add the option we talked about in the meeting briefly, that sigma hierarchical parameters are plotted in LOG10 scale by default. But it takes some manual finding which parameters are sigma parameters in the inner subproblems which can contain noise parameters, then rescaling all the estimated values and its bounds to LOG10. Not the nicest, but it works. For reference, lines 536 to 558 in Can leave and can remove, what do you think? |
Allow for 'log' and 'log10' parameter scales of scaling and offset hierarchical parameters. This does not change anything in their inner optimizations, as that's done analytically, but it allows for better visualization of their values in parameter plots together with all other (mostly log-scaled) parameters. I added a Boehm example below.
Currently not supported for hierarchical sigma parameters as they cannot have different parameter bounds than (0, +inf). So log-scale of its lower bound is -inf causing issues in petab.lint checks. It would be possible to fix, but would require some larger restructuring of when the petab.lint is applied, removing hierarchical sigma parameters from the parameter table before the lint or changes in petab.