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.

Sign up for GitHub

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

Jump to bottom

Add option hide_optimal and correct index for legenda color #66

Merged
merged 7 commits into from
Nov 7, 2024
Merged

Add option hide_optimal and correct index for legenda color #66

merged 7 commits into from
Nov 7, 2024

Conversation

EmilyWes
Copy link
Contributor

@EmilyWes EmilyWes commented Oct 4, 2024

The option is not yet used in the current templates, but for future templates and easier adjusting I added the option the same way as hide_random.

@EmilyWes EmilyWes requested a review from J535D165 October 4, 2024 11:59
J535D165
J535D165 reviewed Oct 4, 2024
View reviewed changes
Copy link
Member

@J535D165 J535D165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Nice to have this option.

It might be good to use ruff to lint the rendered file and see if there are issues.

Can you also render the examples that make use of this template?

@J535D165 J535D165 changed the title Issue_65: Added option hide_optimal and corrected index for legenda color Add option hide_optimal and corrected index for legenda color Oct 4, 2024
@J535D165 J535D165 changed the title Add option hide_optimal and corrected index for legenda color Add option hide_optimal and correct index for legenda color Oct 4, 2024
@J535D165 J535D165 linked an issue Oct 4, 2024 that may be closed by this pull request
Optimal line causes legenda to have grey/black lines #65
Closed
@Rensvandeschoot
Copy link
Member

Solves #65
Integrates asreview/asreview-insights#68
Requires ASReview-insights version 1.3

@Rensvandeschoot Rensvandeschoot mentioned this pull request Oct 6, 2024
Merged
@EmilyWes
Copy link
Contributor Author

EmilyWes commented Oct 7, 2024

Thanks @Rensvandeschoot for the additional info.
@J535D165, I rendered the template by using Makita with an example project, that all works.

@J535D165
Copy link
Member

J535D165 commented Oct 7, 2024

Can you push the changes to the examples as well? Thanks

@J535D165
Copy link
Member

J535D165 commented Oct 7, 2024

I'm a bit lost in all the versioning chaos with Makita atm. But we have to bump the version in the rendered README isn't it? Can you do that as well?

@Rensvandeschoot
Copy link
Member

I added the dependency on Insights 1.3 or higher

J535D165
J535D165 requested changes Oct 15, 2024
View reviewed changes
@jteijema jteijema self-requested a review October 31, 2024 14:02
jteijema
jteijema approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@jteijema jteijema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider upping to insights 1.3.1 since thats the first available version?

@jteijema jteijema requested a review from J535D165 October 31, 2024 14:03
@jteijema @timovdk
Update asreviewcontrib/makita/templates/script_get_plot.py.template
b504575 
Co-authored-by: Timo van der Kuil <5330531+timovdk@users.noreply.github.com>
@jteijema
Copy link
Member

jteijema commented Nov 7, 2024

Thanks all!

@jteijema jteijema merged commit b5cefd6 into asreview:main Nov 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timovdk timovdk timovdk left review comments

@Rensvandeschoot Rensvandeschoot Rensvandeschoot left review comments

@jteijema jteijema jteijema approved these changes

@J535D165 J535D165 Awaiting requested review from J535D165

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimal line causes legenda to have grey/black lines
5 participants
@EmilyWes @Rensvandeschoot @J535D165 @jteijema @timovdk