Skip to content

Conversation

@JeffreySarnoff
Copy link
Contributor

these functions are available

fneg, fabs, fcopysign,
feq, fne, fle, flt,
fadd, fsub, fmul, fdiv, frem,
fmuladd, ffma

no additional tests were added

@codecov
Copy link

codecov bot commented Sep 26, 2021

Codecov Report

Merging #20 (09425bc) into master (baa9ee4) will decrease coverage by 4.35%.
The diff coverage is 23.07%.

❗ Current head 09425bc differs from pull request most recent head 73c31c4. Consider uploading reports for the commit 73c31c4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage   98.33%   93.98%   -4.36%     
==========================================
  Files           7        7              
  Lines         421      449      +28     
==========================================
+ Hits          414      422       +8     
- Misses          7       27      +20     
Impacted Files Coverage Δ
src/float.jl 77.65% <23.07%> (-20.99%) ⬇️
src/int.jl 100.00% <0.00%> (ø)
src/bool.jl 100.00% <0.00%> (ø)
src/tuples.jl 100.00% <0.00%> (ø)
src/ndindex.jl 93.50% <0.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baa9ee4...73c31c4. Read the comment docs.

@chriselrod
Copy link
Collaborator

chriselrod commented Sep 26, 2021

I recently was asked to remove Base.@pure from Pumas.
I asked Jameson about the use there (which was fairly similar to the uses here), and he suggested getting rid of it is a good idea.

Maybe we should make a separate PR, and like in https://github.com/SciML/Static.jl/pull/19/files rely on @inferred tests to trust in the results.

We'll need to set up some benchmarks, though, because inference tends to seem non-deterministic, so it seems like there's always a chance we'll end up getting inference failures in real code.

@Tokazama
Copy link
Collaborator

I second the sentiments on @pure.

@JeffreySarnoff
Copy link
Contributor Author

I am happy to remove the @pure everywhere [I agree that it is inappropriate, however in this case I chose to copy the practice as shown -- no harm, no problem].

@JeffreySarnoff
Copy link
Contributor Author

There remains adding coverage of the new float functions. Following that, I agree with @chriselrod; the more facile approach lets this PR be well-defined and focused on provision of the float intrinsics. We expect to open a second PR once this is complete, to use this revision as the underpinning for the more involved testing with @inferred. Along with that, the repository should have some careful benchmarking to apply as a chooser or refuser of plausibly faster-stabler-memlesser refinement .

@Tokazama
Copy link
Collaborator

Benchmarks are the key thing here. @inferred tests are used pretty aggressively here.

@JeffreySarnoff
Copy link
Contributor Author

JeffreySarnoff commented Sep 26, 2021

What would you like the benchmarking to reveal?
Are there current questions about the relative performance of alternate approaches?
What are the methods of interest for programming these functions, each to be benchmarked?

I am just jumping in; please share implementation mechanics to be examined and compared.

@Tokazama
Copy link
Collaborator

I may have been a bit misleading when I said "Benchmarks are the key thing here". By "here" I meant in Static.jl. I don't think anything in this PR would have questionable performance since it should all be done at compile time.

My concern with performance in #19 is b/c instead of doing Base.:(*)(x::Int, y::StaticInt{N}) where {N} = x * N we aren't specializing on y, so it is more like Base.:(*)(x::Int, @nospecialize(y::StaticInt) where {N} = x * getfield(typeof(x), :parameters)[1], but we still want it to be as fast and inferrible as Base.:(*)(x::Int, y::Int). I haven't been able to detect any performance issues with this yet, but occasionally we'll have an odd issue come up with a static type when it is passed through many methods. For example, deeply nested method calls on StaticInt{N} occasionally won't be inferrible.

We should probably just have a separate PR for that entirely because I assume the end produce will be some tricky micro benchmarks and odd corner cases.

@JeffreySarnoff
Copy link
Contributor Author

I am closing this and replacing it with several simpler successive PRs.
(a) remove @pure
(b) add other float intrinsics with simple sanity tests
(c) provide benchmarking

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.

3 participants