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

Improve _ctreeviz_univar() #227

Merged
merged 9 commits into from Jan 5, 2023
Merged

Improve _ctreeviz_univar() #227

merged 9 commits into from Jan 5, 2023

Conversation

mepland
Copy link
Collaborator

@mepland mepland commented Dec 30, 2022

  • Compute bins only when needed
  • Fix crash for 0 count bins
  • Fix split heights changing in for loop
  • Make new 'preds' key in show param to separately control the horizontal prediction bars
  • Add ValueError for unrecognized gtype
  • Match other rectangle rect_edge color style

image

@mepland
Copy link
Collaborator Author

mepland commented Dec 30, 2022

  • Added tesselation_alpha to let user make horizontal 'preds' bars transparent if they wish
    image

@mepland mepland mentioned this pull request Dec 30, 2022
Closed
@parrt parrt added the enhancement New feature or request label Dec 30, 2022
@parrt parrt added this to the 2.1 milestone Dec 30, 2022
@parrt
Copy link
Owner

parrt commented Dec 30, 2022

The prediction bars look like they are overlapping the bottom of the bar charts. Is this the previous behavior and I just didn't notice? dang. This PR looks fine, but let's leave this open until we can resolve this issue I just raised. Seems like the prediction should go below the axis not on top of the others.

@mepland
Copy link
Collaborator Author

mepland commented Jan 4, 2023 •
edited

The prediction bars look like they are overlapping the bottom of the bar charts. Is this the previous behavior and I just didn't notice? dang. This PR looks fine, but let's leave this open until we can resolve this issue I just raised. Seems like the prediction should go below the axis not on top of the others.

@parrt 3a72aa2 moves the 'preds' boxes below the regular x-axis, which is then redrawn. What do you think?

image

@parrt
Copy link
Owner

parrt commented Jan 5, 2023

What do you think?

looks great!

@parrt
Merge branch 'dev' into improve_ctreeviz_univar
8db4ea5 
Signed-off-by: Terence Parr <parrt@antlr.org>
@parrt parrt changed the base branch from dev to master January 5, 2023 19:26
@parrt parrt changed the base branch from master to dev January 5, 2023 19:27
@parrt parrt merged commit e84511b into parrt:dev Jan 5, 2023
@mepland
Copy link
Collaborator Author

mepland commented Jan 5, 2023

@parrt I think we should remove the tesselation_alpha parameter, now that overlaps are impossible. I can do it in a new branch and PR.

@mepland mepland deleted the improve_ctreeviz_univar branch January 5, 2023 22:05
@parrt
Copy link
Owner

parrt commented Jan 5, 2023

That sounds right. Thank you

@mepland
Copy link
Collaborator Author

mepland commented Jan 5, 2023

That sounds right. Thank you

Done, see #235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parrt parrt parrt left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mepland @parrt