Skip to content

Make prefix more type-stable #121

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make prefix more type-stable #121

wants to merge 2 commits into from

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Apr 19, 2025

While trying to hunt down type instabilities in TuringLang/DynamicPPL.jl#892 this was one of the things that I first looked at. I thought that the changes in this PR would make a difference. However, simply testing @inferred AbstractPPL.prefix(vn1, vn2) with various combinations of vn1 and vn2, I can't actually see any difference on Julia 1.11.5, and it didn't help with any upstream type instabilities.

It appears to me that the compiler is smart enough to figure out that getoptic(vn) returns the second type parameter of vn, and thus when it looks at the line if getoptic(vn) == identity it can statically determine whether this is true or false, and infer the return type appropriately.

I don't know if it's risky to rely on this behaviour, hence why I'm keeping this PR open.

Copy link

codecov bot commented Apr 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.02%. Comparing base (87e3120) to head (88e9a95).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   90.93%   91.02%   +0.09%     
==========================================
  Files           2        2              
  Lines         298      301       +3     
==========================================
+ Hits          271      274       +3     
  Misses         27       27              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coveralls
Copy link

coveralls commented Apr 19, 2025

Pull Request Test Coverage Report for Build 14550376651

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 91.03%

Totals Coverage Status
Change from base Build 14321492427: 0.09%
Covered Lines: 274
Relevant Lines: 301

💛 - Coveralls

@yebai
Copy link
Member

yebai commented Apr 21, 2025

It's probably always a good idea to make the compiler's life easier where possible.

@yebai yebai requested a review from sunxd3 April 21, 2025 18:13
Copy link
Member

@sunxd3 sunxd3 left a comment

Choose a reason for hiding this comment

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

it looks fine to me and I don't see a downside for this over the original implementation.

Regarding fixing type stability errors, I has couple of thoughts, none of them are too serious:

  1. It appears to me that the compiler is smart enough to figure out that getoptic(vn) returns the second type parameter of vn, and thus when it looks at the line if getoptic(vn) == identity it can statically determine whether this is true or false, and infer the return type appropriately.

    is it possible to test the behavior of the compiler in some way? (I am thinking inspecting IRCode)

  2. (I don't have a good enough knowledge of the Julia compiler.) Topic_prefix is nested, right? Do we have an examples of reasonably deeply nested VarName? So that we can know that the compiler doesn't give up too easily?

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.

4 participants