-
Notifications
You must be signed in to change notification settings - Fork 51
Implement Base.instantiate - take 2
#1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: breaking
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1118 +/- ##
===========================================
Coverage ? 82.66%
===========================================
Files ? 57
Lines ? 5631
Branches ? 0
===========================================
Hits ? 4655
Misses ? 976
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I tried Rasters tests on this branch locally and it doesn't seem to break anything. But Rasters tests are broken right now, and there are some other things on this branch that broke it, so I'm not 100% sure. |
|
yeah, maybe lets fix those tests first. I guess its the threading on 1.12? |
The threading is one thing and then the other breaking changes, like removing |
|
I just ran Rasters test to locally with this branch, rafaqz/Rasters.jl#1030, rafaqz/Rasters.jl#1029 and some very small local changes to dispatch on The only failure is with some I/O things with Rasters filed with strings - I think that might be because I'm on windows and I am sure it has nothing to do with this PR. So this is safe to merge AFAIC |
|
Hmm I will wait until you check on linux we do use broadcasts in write |
|
Okay actually the same test does pass on main, so I have to investigate a little more. The thing is I branched this of breaking so there are a whole bunch of breaking changes in here and any of them might cause an error. This is the test that is failing: |
|
It was actually a bug (though pretty niche) - see JuliaIO/DiskArrays.jl#284 |
I'm having another go at this, I'll run Rasters tests as well this time to make totally sure we're not breaking anything.
#917 introduced a
BasicDimensionalStylewhich I don't think we need at all because we can just letsimilardispatch do the work.