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

Fix PCSG handling #104

Merged
merged 9 commits into from
Feb 26, 2025
Merged

Fix PCSG handling #104

merged 9 commits into from
Feb 26, 2025

Conversation

THinnerichs
Copy link
Member

@THinnerichs THinnerichs commented Feb 17, 2025

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 61.25%. Comparing base (beb1b25) to head (3f546a5).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
src/csg/probabilistic_csg.jl 75.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   56.45%   61.25%   +4.79%     
==========================================
  Files           7        7              
  Lines         542      542              
==========================================
+ Hits          306      332      +26     
+ Misses        236      210      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@THinnerichs THinnerichs self-assigned this Feb 17, 2025
@THinnerichs THinnerichs requested a review from ReubenJ February 17, 2025 13:20
Copy link
Member

@ReubenJ ReubenJ left a comment

Choose a reason for hiding this comment

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

Not sure on #102, because other methods currently display a warning and assume uniform probabilities.

If you want to add an extra Base.show method for probabilistic grammars, we can either define this in Core or define it over CSGs instead of AbstractGrammars.

@THinnerichs
Copy link
Member Author

Will discard prettier Base.show for now.

@THinnerichs THinnerichs marked this pull request as ready for review February 19, 2025 15:37
@THinnerichs THinnerichs requested a review from ReubenJ February 19, 2025 15:37
Copy link
Member

@ReubenJ ReubenJ left a comment

Choose a reason for hiding this comment

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

With the tests for non-probabilistic cases, this looks good to me 👍

@THinnerichs THinnerichs requested a review from ReubenJ February 24, 2025 07:29
@ReubenJ ReubenJ merged commit 408e1e4 into master Feb 26, 2025
5 checks passed
@ReubenJ ReubenJ deleted the fix_pcsgs branch February 26, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants