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 many things reported by JET #152

Merged
merged 11 commits into from
Sep 14, 2024

Conversation

lgoettgens
Copy link
Contributor

@lgoettgens lgoettgens commented Sep 12, 2024

Resolve #130.

  • [n/a] The code is properly formatted and commented.
  • [n/a] Substantial new functionality is documented within the docs.
  • [n/a] All new functionality is tested.
  • All of the automated tests on github pass.
  • We recently started enforcing formatting checks. If formatting issues are reported in the new code you have written, please correct them.

I was able to bring the number of reports down to 33 on julia 1.10. On julia 1.11.0-rc3, there are only 19 left. Many of the remaining things are due to some iterator collection magic in julia base, where I at some point gave up trying to understand it.

What will certainly help with the JET report and type stability in general, but what I see out of scope of this small project:

  • Use some type for time and restrict many of the type signatures for it to Union{Nothing, TimeType}. It really does not matter if one uses Float64 or DateTime or whatever for it, but this would help the compiler in some places.
  • Resumable functions currently don't allow the compiler to do small union splitting. Consider the following function
function foo(x::Union{Nothing, Int})
  if isnothing(x)
    error("x is nothing")
  end
  # x gets inferred as Int
  ...
end

If we now tag this function as resumable, x will still be referred as Union{Nothing, Int} there. For the places that came up I circumvented this by introducing a new variable that just has type Int.

@lgoettgens lgoettgens marked this pull request as ready for review September 12, 2024 16:03
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.17%. Comparing base (4eeb57a) to head (a1e7672).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/states_registers_networks_getset.jl 0.00% 3 Missing ⚠️
src/backends/clifford/should_upstream.jl 0.00% 1 Missing ⚠️
src/baseops/traceout.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   73.03%   73.17%   +0.13%     
==========================================
  Files          41       41              
  Lines        1732     1737       +5     
==========================================
+ Hits         1265     1271       +6     
+ Misses        467      466       -1     

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

@Krastanov Krastanov added the Skip-Changelog label for control of CI: skips the changelog check label Sep 14, 2024
@Krastanov
Copy link
Member

Thank you so much! Please send me an email at [email protected] with your name and email and a link to the bounty, so I can set up your reimbursement process with NumFOCUS (it might take a few weeks as we do them in batches and they are still setting everything up for their first batch).

@Krastanov Krastanov merged commit dd84c35 into QuantumSavory:master Sep 14, 2024
12 of 13 checks passed
@lgoettgens lgoettgens deleted the lg/JET-fixes-2 branch September 14, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog label for control of CI: skips the changelog check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve static analysis tests with JET [$200]
2 participants