Skip to content

Reach full test coverage and only support Julia 1.6 onwards - #350

Open
PatrickHaecker wants to merge 3 commits into
JuliaMath:masterfrom
PatrickHaecker:coverage
Open

PatrickHaecker wants to merge 3 commits into
JuliaMath:masterfrom
PatrickHaecker:coverage

Conversation

@PatrickHaecker

@PatrickHaecker PatrickHaecker commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

This PR brings the codecov coverage (hopefully) to 100% while keeping the code clean and efficient.

  • New tests for paths no test reached: reductions over empty and one-element arrays, checked_mul for f == 0, the generic AbstractFloat conversion of Normed, hashing of Normed (through decompose), % of a foreign Real type into Normed, and the deprecated floattype fallback.
  • Require Julia 1.6. No CI job has tested an older version for a long time. This removes code that no tested version could run (copysign/flipsign and Base.showarg workarounds, the precompilation version check). It also removes workarounds that Julia 1.6 no longer needs: @exp2 and the @generated inv_rawone (the generated LLVM code is identical on Julia 1.6, 1.10 and 1.13), the signed/unsigned stubs (a MethodError either way) and the <0.0.1 compat alternative for Random. CI now tests Julia 1.6.
  • Remove an unreachable return in Float32(::Normed{UInt8}). f is at most 8, so the final 0.0f0 could never be returned.

The full test suite passes on Julia 1.6, 1.10 and 1.13.

Update:
This PR is now based on #365, so it will be a single commit once #365 is merged.

@codecov

codecov Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.87%. Comparing base (d62abf3) to head (962d561).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   96.75%   99.87%   +3.12%     
==========================================
  Files           7        7              
  Lines         800      786      -14     
==========================================
+ Hits          774      785      +11     
+ Misses         26        1      -25     

☔ View full report in Codecov by Harness.
📢 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.

@kimikage

kimikage commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

I think it would be better to split this PR into several smaller ones.
While I believe updating to julia v1.6 (or v1.10) is something we should eventually do, I do not think it should be done midway through the FPN v0.9 series.
At the very least, I don't think that aiming for 100% code coverage is a sufficient reason for making the change. In other words, this PR title is somewhat misleading.

The deprecated floattype fallback will soon be removed in tackling #342 (in FPN v0.9).

@kimikage
kimikage marked this pull request as draft October 10, 2026 06:31
@kimikage

Copy link
Copy Markdown
Collaborator

Since @exp2 is a performance issue, I think it is fine to change it regardless of the supported versions.
However, I don't think it is a significant obstacle to maintenance.

@PatrickHaecker PatrickHaecker changed the title Reach full test coverage Reach full test coverage and only support Julia 1.6 onwards Oct 10, 2026
@PatrickHaecker

Copy link
Copy Markdown
Collaborator Author

I think it would be better to split this PR into several smaller ones.

I already did the best split I could come up with in 3 commits. So the recommendation (sorry for forgetting to state it) is to review commit-by-commit. I can split the 3 commits into 3 PRs if that brings advantages I haven't seen. At least one would be very small. So without better suggestions I could reasonably go to 2. But its containing a lot of search-and-replace, so I don't expect a larger number of review findings which seem to make this useful (but again, I might have missed something).

While I believe updating to julia v1.6 (or v1.10) is something we should eventually do, I do not think it should be done midway through the FPN v0.9 series.

It's common to do so. Everything before Julia 1.10 is no longer supported anyway, so it does not make any difference regarding Semver and is therefore no incompatible change. The incompatible change, if you want to call it that way, was done by dropping support for older Julia versions by Julia itself, but as that has been made transparent by Julia, I think that's all fine.

I think the state with this PR is much better than before. Before the PR the package indicated to support 10 Julia versions (1.0 to 1.9) before the oldest one which was tested (1.10)! In other words, I think updating to Julia 1.6 (or 1.10) is the only reasonable thing to do. This would be aligning FixedPointNumbers.jl with packages like Revise.jl or StaticArrays.jl which also do not support older Julia versions than 1.6 (and often dropped everything which is no longer supported anyway).

It's also not as if people actually using such old Julia versions would no longer have FixedPointNumbers available. They just no longer get newer versions, but stick to the older versions. But that does not really matter, as most of the packages in their environment dropped support for these old Julia versions already, so most users couldn't upgrade to newer versions of FixedPointNumbers anyway due to conflicts in their environment.
I can't really upgrade to version FixedPointNumbers 0.9 with Julia 1.13 in anything except toy environments for that very reason.

At the very least, I don't think that aiming for 100% code coverage is a sufficient reason for making the change. In other words, this PR title is somewhat misleading.

But that was indeed the major breakthrough. Only by dropping support for older Julia versions could I reach the 100% coverage. Whether 100% coverage is a valuable goal itself is debatable and whether it's a good enough reason to drop support for older Julia versions is so, too.
However, 100% coverage has the great benefit that it's immediately obvious when a PR adds something uncovered – even when you only pay attention after merging the PR it's quite clear.
Additionally, the main reason is indeed the support argument. However, you can counter that, that you don't drop support when there is not an actual technical reason. Then you get a chicken-egg problem which this PR tries to break through.

The deprecated floattype fallback will soon be removed in tackling #342 (in FPN v0.9).

Good to know. The fix for #342 can then remove the test case when being merged after #350. And the author of that PR does not need to think about whether the PR works for Julia 1.0 when being merged after #350. So this is a good example of how the current PR simplifies development.

@PatrickHaecker
PatrickHaecker marked this pull request as ready for review October 10, 2026 10:03
@kimikage

Copy link
Copy Markdown
Collaborator

Thank you for your thoughtful comments.
I understand your point of view, but I still do not plan to merge this in FPN v0.9.

Regarding the additional tests, I will not review them in detail, but I would like you to commonize the implementation between Fixed and Normed as much as possible.

@PatrickHaecker

PatrickHaecker commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thank you for your thoughtful comments. I understand your point of view, but I still do not plan to merge this in FPN v0.9.

I thought about it some more before I have seen your reply, so I might as well quickly note it down (also for my future me looking it up here):
The only reason we mark backwards-incompatible package version updates with v0.n+1 or vn+1.0 is to avoid that users who Pkg.update get a package which breaks their code. This couldn't happen in such a case when no incompatible change is indicated. On Julia >= 1.6 the update would just work and in Julia < 1.6, the update would not happen due to the incompatible constraint on the Julia version. Just found: SciML also argues like this.

But, ok, thanks to your nice reply. I got it, so let's release it as v0.10. This will create unnecessary work for package authors whose package depends on FixedPointNumbers.jl. But as a lot of packages have not yet done the transition to v0.9 anyway, the faster we release this as v0.10, the less additional work it will create.

Regarding the additional tests, I will not review them in detail, but I would like you to commonize the implementation between Fixed and Normed as much as possible.

Done. I haven't managed to generalize all, but I think that's what you meant with "as much as possible".

Patrick Häcker added 3 commits October 18, 2026 04:07
Covers reductions over empty and one-element arrays, `checked_mul` for
`f == 0`, the generic `AbstractFloat` conversion of `Normed`, hashing of
values that equal a `Rational` (which goes through `decompose`), `%` of a
foreign `Real` type, and the deprecated `floattype` fallback.

The reductions, the hashing and `%` behave the same for `Fixed` and
`Normed`, so common test functions check them over many raw types and
fractional bits.
`Normed{UInt8,f}` requires `1 <= f <= 8`, so after the checks for
`f <= 7` only `f == 8` remains and the final `0.0f0` could never be
returned.
No CI job has tested a Julia version before 1.6 for a long time. Dropping
them removes code that no tested version could run: the workarounds for
`copysign`/`flipsign` with an unsigned sign argument (Julia 1.0 and 1.1),
for `Base.showarg` of arrays (before 1.6) and for precompilation (1.0).

Julia 1.6 also makes further workarounds unnecessary. `@exp2` and the
`@generated` `inv_rawone` only forced constant folding, and plain `exp2`
and division compile to identical code on Julia 1.6, 1.10 and 1.13. The
`signed`/`unsigned` methods only kept Julia 1.0 from converting to an
integer, and without them both still throw a `MethodError`. `Random = "1"`
resolves on Julia 1.6, so the `<0.0.1` alternative is not needed.

CI now tests Julia 1.6. The version becomes 0.10.0, because releases
for older Julia versions end here.
@PatrickHaecker

Copy link
Copy Markdown
Collaborator Author

I think it would be better to split this PR into several smaller ones.

Done. Please review #365, @kimikage.

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