Linrange scaling #294
Linrange scaling #294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
=========================================
+ Coverage 47.71% 47.8% +0.09%
=========================================
Files 21 21
Lines 1071 1071
=========================================
+ Hits 511 512 +1
+ Misses 560 559 -1
Continue to review full report at Codecov.
|
|
I think tests don't fail because your very own fix from within this PR is applied. You should run your tests against master to make it fail. But I think it's a good sign that tests don't fail. That means any previously intended functionality is preserved by your modification, and your fix works. |
|
@dkarrasch that would be great - but the first commit in the PR that introduces the test but not the fix still causes no failures, so I haven't really covered it 😞 |
|
I didn't read carefully enough. It should be in the test. You missed to replace the |
| rescale_gradient(r::StepRangeLen, g) = g / step(r) | ||
| coordlookup(r::AbstractRange, x) = (x - first(r)) / step(r) + oneunit(eltype(r)) | ||
| boundstep(r::AbstractRange) = 0.5*step(r) | ||
| rescale_gradient(r::AbstractRange, g) = g / step(r) |
There was a problem hiding this comment.
Are there potentially more instances that need a consistent change? Something like rescale_hessian for instance?
There was a problem hiding this comment.
Hmm. AFAICT no, rescale_hessian_components, for example, uses steps, which accepts any AbstractRange subtype. We should not have changed any behaviour. It presumably can't hurt to test the hessian scaling also though.
There was a problem hiding this comment.
There are now tests for hessians specifically.. I'm not sure if they are useful tests.
tests/scaling.jl also tests for iteration behaviour and other float types when interpolating; but I don't think we especially want to clone those also for LinRange
test/scaling/scaling.jl
Outdated
|
|
||
| itp = interpolate(1:1.0:10, BSpline(Linear())) | ||
|
|
||
| sitp = @inferred(scale(itp, LinRange(-3,1.5,91))) |
There was a problem hiding this comment.
This duplicates most of the previous test but with a different LinRange.
possibly this is wasteful? But it does fail without the fix as desired.
| rescale_gradient(r::StepRangeLen, g) = g / step(r) | ||
| coordlookup(r::AbstractRange, x) = (x - first(r)) / step(r) + oneunit(eltype(r)) | ||
| boundstep(r::AbstractRange) = 0.5*step(r) | ||
| rescale_gradient(r::AbstractRange, g) = g / step(r) |
There was a problem hiding this comment.
There are now tests for hessians specifically.. I'm not sure if they are useful tests.
tests/scaling.jl also tests for iteration behaviour and other float types when interpolating; but I don't think we especially want to clone those also for LinRange
|
I'll rebase and squash these if they make sense. |
For this to be a sensible commit to fix #293 , my changes to the test suite should actually cause a test failure. However, they don't; I'm doing it wrong. Hints gratefully accepted.