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

Bugfix ignore appear #113

Closed
wants to merge 2 commits into from
Closed

Bugfix ignore appear #113

wants to merge 2 commits into from

Conversation

cmalinmayor
Copy link
Contributor

@funkey There were two issues with the previous implementation.

  • Previously we skipped the appear cost if the attribute was present and False, instead of True as the name implied and the docstring stated.
  • I also had to add the dummy variable with weight of 0.0 to avoid an ilpy error - let me know if that shouldn't be necessary, but I think it is?

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base (951efe7) to head (56c6636).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #113   +/-   ##
=======================================
  Coverage   92.57%   92.58%           
=======================================
  Files          31       31           
  Lines         795      796    +1     
=======================================
+ Hits          736      737    +1     
  Misses         59       59           

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

@cmalinmayor cmalinmayor closed this Aug 1, 2024
@cmalinmayor
Copy link
Contributor Author

Okay after a closer look, my first bullet point above was clearly incorrect. I'm still not convinced that the behavior we see using the ignore appear cost is correct, though. It is skipping a lot of points in the first frame even with the ignore attribute set.

@cmalinmayor cmalinmayor deleted the bugfix-ignore-appear branch November 5, 2024 19:23
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.

1 participant