JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
74 stars 38 forks source link

Revert to zero dependencies #76

Closed mcabbott closed 2 years ago

mcabbott commented 2 years ago

Until recently this package had no dependencies at all. It defines rules for SpecialFunctions and bounds the version needed, but does not load the package. I see no reason that the same mechanism can't be used for LogExpFunctions.

I didn't notice this in #69, and I see no discussion of this change there, everyone was focused on other issues. I think that moving away from zero deps is something that we need a positive reason to do. Until we have one, keeping the old behaviour seems preferable.

Test failures for 1.7 aren't new, or are new only in that 1.7 is the new 1, not caused by this PR:

100
check rules: Test Failed at /home/runner/work/DiffRules.jl/DiffRules.jl/test/runtests.jl:54
101
  Expression: isapprox(dy, finitediff((z->begin
102
                Base.mod(foo, z)
103
            end), bar), rtol = 0.05)
104
   Evaluated: isapprox(-4.0, 33023.599675503; rtol = 0.05)
105
Stacktrace:
codecov-commenter commented 2 years ago

Codecov Report

Merging #76 (5b797c3) into master (b6301e8) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #76   +/-   ##
=======================================
  Coverage   95.74%   95.74%           
=======================================
  Files           2        2           
  Lines         141      141           
=======================================
  Hits          135      135           
  Misses          6        6           
Impacted Files Coverage Δ
src/rules.jl 99.23% <100.00%> (ø)

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 b6301e8...5b797c3. Read the comment docs.

devmotion commented 2 years ago

I'm not against this PR but I want to point out that

Until recently this package had no dependencies at all.

is incorrect. The package depends on SpecialFunctions and NaNMath since Project.toml was added in 0.1.0: https://github.com/JuliaDiff/DiffRules.jl/blob/v0.1.0/Project.toml

Therefore I followed the same approach in the LogExpFunctions PR (which was already an indirect dependency through SpecialFunctions) and added LogExpFunctions as a direct dependency.

mcabbott commented 2 years ago

Yes, the quoted sentence is a bit ambiguous, but the next sentence explains. There are two meanings to dependencies, one is having things in Project.toml (which bounds versions and forces them to be downloaded) and the other is loading them (which needs to run all the packages). The second is strictly more brittle than the first, as packages which error on loading can cause things to fail.

In the first sense, the package does and did have dependencies, as you say.

This PR addresses only the second sense.

devmotion commented 2 years ago

the other is loading them (which needs to run all the packages). The second is strictly more brittle than the first, as packages which error on loading can cause things to fail.

If a package fails to load it means the compat entry is wrong and it should not be possible to install it in the first place, it seems? It should be possible to load all package versions that are stated to be compatible.

cossio commented 2 years ago

I agree with the approach in this PR. Having to manually check isdefined for each new function seems like re-implementing by hand what the compat bound is meant to accomplish in the first place.

If a package fails to load it means the compat entry is wrong and it should not be possible to install it in the first place, it seems? It should be possible to load all package versions that are stated to be compatible.

Not necessarily. There could be a bug in the dependency, which gets corrected in the next patch release (of the dependency). In this case, the compat entry here is correct.

mcabbott commented 2 years ago

Yes. That was the case with the (fairly) recent Static.jl apocalypse -- some internal macro it was using changed, and thus about 1000 packages could no longer load the its code, until this was fixed. But a package which simply downloaded its code would have been unaffected.

cossio commented 2 years ago

CI passes here. Merge?

cossio commented 2 years ago

Friendly bump. Can we merge this?