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

Refinements to display_violinplot_thicket Function #156

Merged
merged 2 commits into from
May 15, 2024

Conversation

Yejashi
Copy link
Collaborator

@Yejashi Yejashi commented May 1, 2024

This PR improves the display_violinplot_thicket function in two ways:

  • Changed default xlabel to 'thicket' for clarity.

  • Restricted function to accept only the same node across all provided thickets.

@Yejashi Yejashi added area-stats Issues and PRs related to Thicket's stats subpackage priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers labels May 1, 2024
@pearce8 pearce8 requested review from ilumsden and pearce8 May 1, 2024 21:46
@ilumsden ilumsden added the type-feature Requests for new features or PRs which implement new features label May 8, 2024
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

Most of this looks good, but there are two main issues that need to be addressed before I can approve.

@Yejashi Yejashi force-pushed the display_violinplot branch from 9ff14bc to 015d3b3 Compare May 13, 2024 06:14
@ilumsden
Copy link
Collaborator

@Yejashi please rebase this PR on #160 or rebase on develop after #160 is merged. There is a bug in the re-exports of display_violinplot.py that is addressed by #160. If you do not merge the changes from this PR, you will reintroduce the bug.

@ilumsden ilumsden self-requested a review May 13, 2024 16:01
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

This looks good. @pearce8 just don't merge until my comment regarding #160 is addressed

@Yejashi Yejashi force-pushed the display_violinplot branch from 015d3b3 to d829771 Compare May 15, 2024 14:29
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.19%. Comparing base (9c53fd9) to head (d829771).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #156      +/-   ##
===========================================
+ Coverage    80.15%   80.19%   +0.03%     
===========================================
  Files           52       52              
  Lines         3442     3448       +6     
===========================================
+ Hits          2759     2765       +6     
  Misses         683      683              
Files Coverage Δ
thicket/stats/display_violinplot.py 65.53% <100.00%> (+0.59%) ⬆️
thicket/tests/test_display.py 100.00% <100.00%> (ø)

@pearce8 pearce8 merged commit 6a7904c into LLNL:develop May 15, 2024
4 checks passed
@slabasan slabasan added this to the 2024.2.0 milestone Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-stats Issues and PRs related to Thicket's stats subpackage priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-feature Requests for new features or PRs which implement new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants