Make gradient fixes #446
Make gradient fixes #446
Conversation
|
It is type unstable. |
|
My sense is that a simple fix might not be possible. We might need a more sophisticated mechanism to decide when the arguments might affect the return type. It did not start with your changes |
Codecov Report
@@ Coverage Diff @@
## master #446 +/- ##
==========================================
+ Coverage 84.81% 84.83% +0.01%
==========================================
Files 25 25
Lines 1646 1648 +2
==========================================
+ Hits 1396 1398 +2
Misses 250 250
Continue to review full report at Codecov.
|
|
Huh. I'll merge it if this works for you. I'll have to study this more closely. |
|
every non-trivial PR should have associated tests |
|
That's a good point. You should add a test for the original issue you cited. |
|
I have no idea what |
There was a problem hiding this comment.
I have no idea what ERROR: Unsatisfiable requirements detected for package FFTW [7a1cc6ca]: means.
This is an issue with the old Pkg version; it fails to resolve the possible version when there is one. You can assist the Pkg resolver by coping this part to the github/workflows/CI.yml configuration file; that basically says: okay I need this version of FFTW, you Pkg do the rest and then it finds out one.
|
Are we picking up a dependency on FFTW? |
No, but ImageCore and some other packages (e.g., Zygote) depend on it. We just need to manually assist the pkg resolving by explicitly adding them during CI initialization for old Julia versions. |
|
It seems quite complicated here because we want the latest version of Zygote for test purposes but it's really julia >=1.3. I see two possible options:
# in julia-1.0
pkg> add FFTW Zygote ImageCore
Updating `~/tmp/environments/v1.0/Project.toml`
[7a1cc6ca] + FFTW v1.1.1
[a09fc81d] + ImageCore v0.9.1
[e88e6eb3] + Zygote v0.4.22
[7a1cc6ca] + FFTW v1.1.1
[a09fc81d] + ImageCore v0.9.1
[e88e6eb3] + Zygote v0.4.22 |
|
That's a bit fiddly CI wise - would Interpolations.jl accept a lower bound to Julia 1.3 or 1.6? What if 1.6 is LTS? |
|
Wow, changing it to |
|
CVS should be in [extras] and not in [deps] Project.toml is designed in a way that we can manually edit it. You can read more on http://pkgdocs.julialang.org/v1/toml-files/ |
|
I will first squash all these previous commits, and then we shall merge. |
|
I have messed up the rebase badly. I'll fix it asap 😢 |
…80da2c334b556 parent 5465957 author SomTambe <[email protected]> 1627561237 +0530 committer SomTambe <[email protected]> 1627561307 +0530 # This is a combination of 6 commits. parent 5465957 author SomTambe <[email protected]> 1627561237 +0530 committer SomTambe <[email protected]> 1627561244 +0530 # This is a combination of 4 commits. # This is the 1st commit message: Resolve commits 1 # This is the commit message #2: Remove ImageCore, add ColorVectorSpace # This is the commit message #3: Add ColorVectorSpace for tests, resolve dependency issues # This is the commit message #4: Shift CVC to [extras] # This is the commit message #5: Rectify silly error # This is the commit message #6: Rectify silly error again # This is the commit message #8: Rectify the final silly error
author SomTambe <[email protected]> 1627561237 +0530 committer SomTambe <[email protected]> 1627561307 +0530 parent 5465957 author SomTambe <[email protected]> 1627561237 +0530 committer SomTambe <[email protected]> 1627561244 +0530 Resolve commits 1 Remove ImageCore, add ColorVectorSpace Add ColorVectorSpace for tests, resolve dependency issues Shift CVC to [extras] Rectify silly error Rectify silly error again Rectify the final silly error Resolve conflicts Rectify the final silly error Resolve conflicts
|
I think the PR is finally good to go! |
This fixes the issue I mentioned in this comment.
I'll add the tests in a bit.