Re observation index selector

[DanSava]

Issue Resolves #346 Resolves #341 Resolves #342 Resolves #343 Resolves #345 Resolves #347 Resolves #348 Resolves #352

[DanSava]

The resulting changes look as follows:

[oysteoh]

Think it looks great! But - i struggle to play with it - nothing happens when i'm clicking / selecting currently. Any clues? 🤔 Nevermind - It works :)

[oysteoh]

Could this change also include updates to the tutorial ? And - i think this looks good and ready for feedback. Not everything is clear and make sense in the beginning, but i like it after playing around. Easier to select the data you care about :)

[DanSava]

Could this change also include updates to the tutorial?

What tutorial are you referring to?

Easier to select the data you care about :)

Currently, only one selection rectangle is allowed but with a bit more work it is possible to allow multiple selection rectangles. But I would suggest that as a future improvement and not be part of the current POC

[oysteoh]

What tutorial are you referring to? The guided tour... I didn't notice it was changed and the new feature was documented ! Well done 👍

[valentin-krasontovitsch]

having one ensemble, one response with a couple observartions, and at least one parameter, when selecting only one observation, and changing which observation is selected by drawing a rectangle around a new one, the correlation x index is not updated in the "active info" line on top, and also not in the scatterplot.

only clicking on the heatmap updates the index and the scatterplot.

is this a bug? should i open an issue for this?

[valentin-krasontovitsch]

I see that handling of corr_x_index was merged back into update_active_resp_param, the docstring of update_corr_index removed, and the docstring of update_active_resp_param not updated - bummer that we lost that refactoring to separate handling of those things. i can see that the current handling of the corr_x_index is simpler, but it also seems be lacking, considering the above bug(?).

perhaps you could consider making it into its own function again, and describing how and when it gets updated.

[valentin-krasontovitsch]

the heatmap info text should perhaps be updated to reflect that the heatmap can be used to change the active x index.

[valentin-krasontovitsch]

we should also consider renaming corr_x_index to something like active_observation_index to reflect that we are really only interested in observations, not random points (and their indexes)

[valentin-krasontovitsch]

i guess it's intentional that only responses with observations are visible in the response correlation viewer now? should that be documented / indicated somewhere?

[valentin-krasontovitsch]

it looks like collapsing the selectors also leads to the observation selector being partly hidden - don't know if this is just me though...

[valentin-krasontovitsch]

another little thing - when sliding the axes of the observation selector, the points under the selection rectangle move, but the selection doesn't get updated. i suppose this is something that can be fixed lated (should i open an issue for that?)

[DanSava]

having one ensemble, one response with a couple observartions, and at least one parameter, when selecting only one observation, and changing which observation is selected by drawing a rectangle around a new one, the correlation x index is not updated in the "active info" line on top, and also not in the scatterplot.

only clicking on the heatmap updates the index and the scatterplot.

is this a bug? should i open an issue for this?

It's a bug should be fixed

[DanSava]

I see that handling of corr_x_index was merged back into update_active_resp_param, the docstring of update_corr_index removed, and the docstring of update_active_resp_param not updated - bummer that we lost that refactoring to separate handling of those things.

I read somewhere that extensive function comments are the promise of future lies. I will update the docsctring for update_active_resp_param to match what is going on there now.

i can see that the current handling of the corr_x_index is simpler, but it also seems be lacking, considering the above bug(?).

perhaps you could consider making it into its own function again, and describing how and when it gets updated.

Not really a good idea to have it separate, more code to read, more code to understand, more code to maintain, to read more comments to understand and keep up to date with changes.

[DanSava]

the heatmap info text should perhaps be updated to reflect that the heatmap can be used to change the active x index.

Not sure what heatmap info you are referring to

[valentin-krasontovitsch]

Not sure what heatmap info you are referring to

the one in the guided tour : )

[DanSava]

i guess it's intentional that only responses with observations are visible in the response correlation viewer now?

Yes

should that be documented / indicated somewhere?

not sure, are you referring to documentation for the user or for the developer?

[valentin-krasontovitsch]

the promise of future lies

yeah i can see how they demand a lot of discipline: have to be read and perhaps updated every time a function is touched...

Not really a good idea to have it separate, more code to read, more code to understand, more code to maintain, to read more comments to understand and keep up to date with changes.

i guess we have very different opinions in this case - i think when i want to figure out how a variable is manipulated, then having a function that is dedicated to just that variable (and not a bunch of variables) makes it easier to read and understand what is going on. also i thought we were trying to avoid returns with several values?

if the functions are clearly structured and named, and the variables are named clearly, precisely, and in a descriptive fashion, then one can perhaps forego the docstring. I've heard several people on the team say that dash and its callback functions are difficult to understand, and it sure as heck was hard for me to figure out what was going on... so yeah the docstrings increase maintenance costs, for sure. my hope is that they also make it easier to get into the code, and start doing changes and verifying behaviour - especially since we don't have any place where the expected behaviour of the app is documented!

yes, after remembering all this, i'm a very very strong advocate for documenting the callback functions with describing the expected behaviour of the app, as in what user interactions should trigger what changes in the views. the technical /implementation details we can skip.

[valentin-krasontovitsch]

not sure, are you referring to documentation for the user or for the developer?

not sure either - perhaps a place where it's obvious for both? maybe the label text of the response selector?

[DanSava]

another little thing - when sliding the axes of the observation selector, the points under the selection rectangle move, but the selection doesn't get updated. i suppose this is something that can be fixed lated (should i open an issue for that?)

I guess this is only for the top axis when there is both an index and a time axis in the same plot and you are sliding the top axis then the points are moving but the rectangle is fixed, correct?

(should i open an issue for that?)

I think it is a good idea to have an issue to disable the sliding of axis.

[valentin-krasontovitsch]

I guess this is only for the top axis when there is both an index and a time axis in the same plot and you are sliding the top axis then the points are moving but the rectangle is fixed, correct?

yes, you're right! funny, didn't realize that.

gonna make an issue for ... disabling the sliding when the top and bottom axis are visible?

[DanSava]

Not sure what heatmap info you are referring to

the one in the guided tour : )

what is not clear about the following?

Heatmap based representation of correlation (-1, 1) among all selected responses and parameters, for currently selected ensembles in column-wise fashion.One can select a currently active response and parameter by clicking directly on a heatmap.

One can select a currently active response and parameter by clicking directly on a heatmap.

I think it is pretty self-explanatory that you can click on the heatmap and that new active response and parameter will be selected

[valentin-krasontovitsch]

what is not clear about the following?

oh no, it's clear, don't get me wrong. sorry, i should have been more verbose - i would add a comment about choosing the active observation (index), as it's the only place where one can jump between chosen observations

[DanSava]

yes, you're right! funny, didn't realize that.

gonna make an issue for ... disabling the sliding when the top and bottom axis are visible?

I would just disable sliding it should not matter if there are both axis or not

[valentin-krasontovitsch]

I would just disable sliding it should not matter if there are both axis or not

coolio!

[DanSava]

yeah i can see how they demand a lot of discipline: have to be read and perhaps updated every time a function is touched...

Not really a good idea to have it separate, more code to read, more code to understand, more code to maintain, to read more comments to understand and keep up to date with changes.

i guess we have very different opinions in this case - i think when i want to figure out how a variable is manipulated, then having a function that is dedicated to just that variable (and not a bunch of variables) makes it easier to read and understand what is going on. also i thought we were trying to avoid returns with several values?

Yes, we have very different opinions in this case.

also i thought we were trying to avoid returns with several values?

That is very true when you write your own logic, but Dash is built with the premise that you should return multiple outputs at the same time, they actually encourage this ,not really sure why, probably because you cannot return the same output from multiple callbacks also might be related to callback order when you have the same input triggering different outputs.

if the functions are clearly structured and named, and the variables are named clearly, precisely, and in a descriptive fashion,

This is very subjective...

I've heard several people on the team say that dash and its callback functions are difficult to understand, and it sure as heck was hard for me to figure out what was going on... so yeah the docstrings increase maintenance costs, for sure. my hope is that they also make it easier to get into the code, and start doing changes and verifying behaviour - especially since we don't have any place where the expected behaviour of the app is documented!

I agree, extensively documenting a page which is marked as experimental just seems excessive given that it will be changed very frequently in the future.

[valentin-krasontovitsch]

Dash is built with the premise that you should return multiple outputs at the same time, they actually encourage this

I see! alrighty then.

extensively documenting a page which is marked as experimental just seems excessive given that it will be changed very frequently in the future.

that's a good point : )

[hnformentin]

Sorry for coming late to the party. It took a while to fix my environment which was broken...

Some observations, not sure if this is the intended behaviour:

1. If I have a response selected with multiple observations selected in the selector, like here:

and I select another response, then I loose track of the observations selected previously:

2. It is not possible to select a time or observation from the response plot (e.g. move the vertical red bar to select a different point).

3. In the observation selector, the axis are movable to the right and the left. Nevertheless, if there is an interval selected and I move the axis, it does not update the selection box. For example, here I had one index selected and two timestamps:

   

But when I moved the index axis (and bring one additional index into the box), the data displayed is not updated.

I think that would be more sensible that the axis was not movable or the data get updated when we move the axis.

[valentin-krasontovitsch]

@hnformentin sorry it's hidden in that wall of text:

• the axis moving is noted, and an issue has been created to disable it
• the selection of a current index / observation is indeed not possible through clicking on the response overview plot anymore, one must / can only click on the heatmap

[hnformentin]

I can continue playing a little bit, please let me know if I should add the comments elsewhere...

[DanSava]

@hnformentin Thank you for taking the time to review

Sorry for coming late to the party. It took a while to fix my environment which was broken...

Some observations, not sure if this is the intended behaviour:

1. If I have a response selected with multiple observations selected in the selector, and I select another response, then I loose track of the observations selected previously:

Yes, that is the intended behavior, what you are doing there is selecting a response with observations that are time indexed and then selecting a response with observations that are value indexed. In this case the observation selection was made only for time index. That is why the selection range is reset when a new response is added and the first observation for each of the responses is considered active.

2. It is not possible to select a time or observation from the response plot (e.g. move the vertical red bar to select a different point).

True, this is by design, only by clicking the heat-map will the selected observation index change.

3. In the observation selector, the axis are movable to the right and the left. Nevertheless, if there is an interval selected and I move the axis, it does not update the selection box. I think that would be more sensible that the axis was not movable or the data get updated when we move the axis.

as @valentin-krasontovitsch already stated this will be addressed in a further issue and the axis moving should be disabled.

[hnformentin]

Nice, it seems you have all in control 👏 👏 👏 Thanks @valentin-krasontovitsch and @DanSava for addressing the comments...