Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expand JET test #782

Merged
merged 2 commits into from
Jan 27, 2025
Merged

Expand JET test #782

merged 2 commits into from
Jan 27, 2025

Conversation

penelopeysm
Copy link
Member

I'm not sure if this is the right thing to do, so pinging @torfjelde:

If JET infers an untyped varinfo then calling test_call later with an untyped varinfo doesn't tell us why it did so - instead we have to call it with a regular typed VarInfo - I think? Let me know if your intent was otherwise:)

(CI on this PR will likely fail, precisely because JET is inferring an untyped varinfo for one of our demo models, there's more explanation here #781 (comment).)

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.16%. Comparing base (727da63) to head (bae04ff).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #782   +/-   ##
=======================================
  Coverage   86.16%   86.16%           
=======================================
  Files          36       36           
  Lines        4301     4301           
=======================================
  Hits         3706     3706           
  Misses        595      595           

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

@coveralls
Copy link

coveralls commented Jan 21, 2025

Pull Request Test Coverage Report for Build 12952966615

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 86.246%

Files with Coverage Reduction New Missed Lines %
src/varinfo.jl 3 83.87%
Totals Coverage Status
Change from base Build 12908348905: 0.0%
Covered Lines: 3706
Relevant Lines: 4297

💛 - Coveralls

@torfjelde
Copy link
Member

Hmm, I don't think I fully understand the change 🤔

The intent with the original test is to the check that determine_suitable_varinfo actually does result in something that can handle both evaluation and sampling.

I think if we want the error itself, then we should just print that information upon test failure instead ,no? I.e. just add an if statement that does this.

@penelopeysm
Copy link
Member Author

penelopeysm commented Jan 24, 2025

Aha, okay, I see what you mean. I was looking at this because there was a case where determine_suitable_varinfo returned an untyped varinfo, but the subsequent call to gen_evaluator_call_with_types (with untyped varinfo) still gave an error: actually there's a run here https://github.com/TuringLang/DynamicPPL.jl/actions/runs/12919450907/job/36029988127#step:6:894

Is that expected?

@torfjelde
Copy link
Member

It's not un-intended 👀 I guess I just didn't think of it, but I do think we want to keep it as is so make sure that the determine_suitable_Varinfo is doing what it's supposed to. Maybe just catch the errors and mark as failed?

@penelopeysm
Copy link
Member Author

Sure, the main thing I think I'd have liked to know was why it didn't determine a typed varinfo. I'm happy to add that in rather than changing this one, I just wasn't sure if that was what this was intended to be heh.
Will edit:)

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

Lovely @penelopeysm !

@penelopeysm penelopeysm added this pull request to the merge queue Jan 27, 2025
@penelopeysm penelopeysm changed the title Fix JET test (?) Expand JET test Jan 27, 2025
Merged via the queue into master with commit 7a140bc Jan 27, 2025
20 checks passed
@penelopeysm penelopeysm deleted the py/jet branch January 27, 2025 13:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants