Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLT - Aggregate similar solvers in legend #474

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Badr-MOUFAD
Copy link
Member

@Badr-MOUFAD Badr-MOUFAD commented Nov 8, 2022

This enhances the plotting part:

  • Aggregate solvers in legend under the same name
  • enable hiding/showing aggregated solvers in legend and plots

Feature preview

next step

  • enhance styling
  • more complete filtering tool

P.S: similar solvers means the same solver run with different parameters

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2022

Codecov Report

Merging #474 (cad1835) into main (4dbec89) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main     #474   +/-   ##
=======================================
  Coverage   54.40%   54.40%           
=======================================
  Files          43       43           
  Lines        2814     2814           
  Branches      516      516           
=======================================
  Hits         1531     1531           
  Misses       1168     1168           
  Partials      115      115           

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
@Badr-MOUFAD Badr-MOUFAD changed the title ENH - Aggregate similar solvers in legend PLT - Aggregate similar solvers in legend Nov 10, 2022
@Badr-MOUFAD
Copy link
Member Author

@tomMoral, I uploaded the feature preview instead.
Have a look at #474 (comment)

@tomMoral
Copy link
Member

When only one is available, I would not create a group no?
image

@tomMoral
Copy link
Member

Also, I liked the previous behavior where the solver tag where all on the same line is not too long :

  • Now
    image

  • Before
    image

@Badr-MOUFAD
Copy link
Member Author

Badr-MOUFAD commented Nov 11, 2022

@tomMoral, I considered your remarks in the last changes
pls have a look on the feature preview #474 (comment)

@bmalezieux
Copy link
Contributor

When only one is available, I would not create a group no?

I think it is better to keep the group structure to preserve homogeneity. Otherwise, the result looks a bit weird (see the Lasso benchmark in #474)

@Badr-MOUFAD
Copy link
Member Author

Badr-MOUFAD commented Nov 12, 2022

I think it is better to keep the group structure to preserve homogeneity. Otherwise, the result looks a bit weird (see the Lasso benchmark in #474)

I managed to restrict the aggregation only for solvers that are run with multiple configurations of parameters
@bmalezieux, could you check whether it renders well for you?

@bmalezieux
Copy link
Contributor

I think it is better to keep the group structure to preserve homogeneity. Otherwise, the result looks a bit weird (see the Lasso benchmark in #474)

I managed to construct groups for solvers that are run with multiple configurations of parameters @bmalezieux, could you check whether it renders well for you?

Actually, I preferred the last version with one block per solver, even with a single configuration. What disturbs me is that the solvers should be represented in the same way in the legend, and this is not the case here in my opinion.

@Badr-MOUFAD
Copy link
Member Author

WDYT @tomMoral?

@tomMoral
Copy link
Member

tomMoral commented Nov 14, 2022

I agree that we should find a consistent way to do this but I don't like the fact that we have to scroll to have the full legend, even when all legend labels could be put on 2/3 lines. This is something we changed 6 months ago so it feels that we are going back.
We need to find a better way to present this than big blocks that stack over each other.

Maybe simply make the parent block also a float, that do not take the full screen. That way, solvers with one config will jsut have an extra gray box above with the name of the solver and multiple solver will have a box around grouping them together? Something like

image

@Badr-MOUFAD
Copy link
Member Author

Badr-MOUFAD commented Nov 18, 2022

WDYT @tomMoral ?

Screenshot from 2022-11-18 19-46-00

links to preview:

@Badr-MOUFAD
Copy link
Member Author

@bmalezieux any thoughts about #474 (comment)?

@bmalezieux
Copy link
Contributor

It looks good ! I think it would be even better to add the name of the solver above solo solvers (it is what Thomas meant if I understand the drawing correctly).

@tomMoral
Copy link
Member

Yes I like the style!! :)
As @bmalezieux said, I would keep the box above the solo solvers.
Also, could you make it so that if we fall a box, it stay the same width?

@bmalezieux
Copy link
Contributor

bmalezieux commented Nov 22, 2022

By the way, I find it clearer to put the style in the css file rather than in the class name.

@Badr-MOUFAD
Copy link
Member Author

Badr-MOUFAD commented Nov 22, 2022

Also, could you make it so that if we fall a box, it stay the same width?

I'm afraid it won't be possible @tomMoral.

Instead, as discussed with @bmalezieux, I made the aggregate of the solver grayish when it's disabled.

Links to to preview:

@mathurinm
Copy link
Contributor

I'm late to the party, but instead of graying the unselected solvers, would it be possible to collapse the unselected block, displaying only its title ? If there are many solver sin a block and they're unselected, one still has to croll a lot to go to the next block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants