dianna-ai / dianna

Deep Insight And Neural Network Analysis
https://dianna.readthedocs.io
Apache License 2.0
48 stars 13 forks source link

816-NEW-dashboard-add-text #855

Closed elboyran closed 1 month ago

elboyran commented 2 months ago

Issue #816

laurasootes commented 2 months ago

@elboyran I like the text for the home page! I left a few small comments on the text.

I also fixed the dashboard tests, they all pass now

elboyran commented 2 months ago

@elboyran I like the text for the home page! I left a few small comments on the text.

I also fixed the dashboard tests, they all pass now

Thanks! I'll keep working in this PR on the remaining task in issue #816 .

laurasootes commented 1 month ago

Hi @elboyran I left a few of comments on the text.

I have one suggestion: with the additional info on the explanation on each page, there is a lot of text when you select an example: first the explanation, then the explanation of the example, the block with the prediction/parameters, and only then comes the explanation: image

what could be an option is to put the description in an expander like this: image The explanation could be standard visible upon opening the page, but once you have loaded your data/example, it is placed inside the expander. What do you think?

laurasootes commented 1 month ago

I just realised that the colorbars have become a bit of a mess with regards to normalisation: In the image example, lime is fine between -1 and 1, but rise and kernelshap are not normalised so range very differently from blue to red. In the tabular example lime is again fine between -1 and 1, but the rise explanation is rescaled to be between -1 and 1 for the weather data, and the frb example does again its own range between blue and red.

Part of this is is because it is differently done in the dianna visualiser per method by default. How would we want it to look like?

elboyran commented 1 month ago

I just realised that the colorbars have become a bit of a mess with regards to normalisation: In the image example, lime is fine between -1 and 1, but rise and kernelshap are not normalised so range very differently from blue to red. In the tabular example lime is again fine between -1 and 1, but the rise explanation is rescaled to be between -1 and 1 for the weather data, and the frb example does again its own range between blue and red.

Part of this is is because it is differently done in the dianna visualiser per method by default. How would we want it to look like?

That's the reason I made issue #856 and it's kind of urgent and blocking.

laurasootes commented 1 month ago

I just realised that the colorbars have become a bit of a mess with regards to normalisation: In the image example, lime is fine between -1 and 1, but rise and kernelshap are not normalised so range very differently from blue to red. In the tabular example lime is again fine between -1 and 1, but the rise explanation is rescaled to be between -1 and 1 for the weather data, and the frb example does again its own range between blue and red. Part of this is is because it is differently done in the dianna visualiser per method by default. How would we want it to look like?

That's the reason I made issue #856 and it's kind of urgent and blocking.

yes I just realised, I did not see it that yet on Friday :). But we should indeed address it in that issue

elboyran commented 1 month ago

Hi @elboyran I left a few of comments on the text.

I have one suggestion: with the additional info on the explanation on each page, there is a lot of text when you select an example: first the explanation, then the explanation of the example, the block with the prediction/parameters, and only then comes the explanation: image

what could be an option is to put the description in an expander like this: image The explanation could be standard visible upon opening the page, but once you have loaded your data/example, it is placed inside the expander. What do you think?

Hi @laurasootes, the reason I left the other text to be always visible is (apart from not knowing how to do this drop down thing ;-) ) that people should know how to read the colormap when seeing it within the example. I agree that limits the visibility, but why would we call it "description explainer"? It's about explainability and relevances in general. Also, as I said I don't know how to do this hiding ;-) BTW I wasn't aware of your review, did you submit it as one?

elboyran commented 1 month ago

Hi @elboyran I left a few of comments on the text.

Hi again, @laurasootes. I cannot seem to find new comments after your first review.

Or do you mean the comments were then, and now (appended) you have your major suggestion to hide the explanation text when starting an example?

BTW I like your suggestion, what if we call of Description of the explanation or Explanation description?

laurasootes commented 1 month ago

Hi @elboyran I left a few of comments on the text.

Hi again, @laurasootes. I cannot seem to find new comments after your first review.

Or do you mean the comments were then, and now (appended) you have your major suggestion to hide the explanation text when starting an example?

BTW I like your suggestion, what if we call of Description of the explanation or Explanation description?

@elboyran the comments above, after you commented 5 days ago and asked me to review, are new and not addressed yet right?

elboyran commented 1 month ago

@cwmeijer did you find out why your PRs notebook checks are failing?

elboyran commented 1 month ago

The macos-related checks are failing, not related to this PR.

laurasootes commented 1 month ago

@elboyran I added the collapsable section, do you like it this way? If so, I think we can merge this

elboyran commented 1 month ago

Hi @laurasootes , I like it a lot.

  1. Do you think it makes sense the description to appear below or maybe better still above the example text (in its "original" place)?
  2. The colormap image now is shifted to the right and looks displaced from the text above. I will try to fix it and maybe remove the end of lines after all as you suggested if that will alight it better ;-)
laurasootes commented 1 month ago

@elboyran

  1. I was wondering the same thing, not sure what makes the most sense. I can move it up if you think that’s better.
  2. I indeed centered the colormap image, I thought that makes sure that the eye of the user is also directed to the instruction below “Select which input type to use in the left panel to continue”
elboyran commented 1 month ago

@elboyran

1. I was wondering the same thing, not sure what makes the most sense. I can move it up if you think that’s better.

2. I indeed centered the colormap image, I thought that makes sure that the eye of the user is also directed to the instruction below “Select which input type to use in the left panel to continue”

@laurasootes

  1. Please, go ahead with moving it up.
  2. Maybe the image should be where it was, and I will think about how to make the instruction more prominent.
laurasootes commented 1 month ago

@elboyran I moved up the texted and removed the centering of the image. In the end the difference is this (also including the difference in yes/no line breaks), I guess it does not matter that much for the instruction with the image on the left.

Screenshot 2024-10-03 at 12 11 34 Screenshot 2024-10-03 at 12 12 10