Skip to content
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

Prepare for removal of FillArrays._copy_oftype #148

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

devmotion
Copy link
Contributor

JuliaArrays/FillArrays.jl#273 plans to remove the internal function FillArrays._copy_oftype since it is not used anymore in FillArrays. It seems ArrayLayouts imports this internal function and hence will be broken if it is not available anymore. This PR fixes the problem by defining the helper function in ArrayLayouts itself if FillArrays._copy_oftype is not available.

Alternatively, a slightly simpler approach would be to unconditionally define _copy_oftype in ArrayLayouts - at the cost of an additional method if FillArrays._copy_oftype exists.

When this PR is merged, we can rerun the downstream tests in JuliaArrays/FillArrays.jl#273 to ensure that the ArrayLayouts master is not broken by the FillArrays PR.

cc: @jishnub

@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (a7786a0) 86.49% compared to head (d5c959e) 86.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage   86.49%   86.52%   +0.03%     
==========================================
  Files          10       10              
  Lines        1740     1744       +4     
==========================================
+ Hits         1505     1509       +4     
  Misses        235      235              
Impacted Files Coverage Δ
src/ArrayLayouts.jl 82.32% <100.00%> (+0.19%) ⬆️
src/mul.jl 82.07% <100.00%> (+0.17%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jishnub
Copy link
Member

jishnub commented Jul 9, 2023

I think it's ok to add it here without referring to FillArrays. Bugfixes may be simpler that way

@devmotion
Copy link
Contributor Author

OK, I simplified the PR 🙂

@jishnub
Copy link
Member

jishnub commented Jul 10, 2023

Could you add some tests for these?

@devmotion
Copy link
Contributor Author

Which tests do you have in mind? I assumed it would already be tested at least indirectly since the function is already used by ArrayLayouts (and hence not "new").

@jishnub
Copy link
Member

jishnub commented Jul 10, 2023

That's possible, and in that case, no further tests are necessary. I just wasn't certain if both the methods were tested. But then the coverage data suggests that it's indeed being tested.

@devmotion
Copy link
Contributor Author

Honestly, it's not clear to me if it was actually tested (indirectly) properly. So I just copied the tests from FillArrays (which are a bit special and FillArrays-specific) and some more general tests.

@devmotion
Copy link
Contributor Author

Good to merge and release @jishnub (I guess it's obvious that I'd like to get the FillArrays PR merged to fix the Turing issues 😛)?

@jishnub
Copy link
Member

jishnub commented Jul 10, 2023

Looks good to me

@jishnub
Copy link
Member

jishnub commented Jul 10, 2023

Could you bump the patch version?

@devmotion
Copy link
Contributor Author

Done 👍

@jishnub jishnub merged commit f69ac2a into JuliaLinearAlgebra:master Jul 10, 2023
15 checks passed
@devmotion devmotion deleted the dw/_copy_oftype branch July 10, 2023 16:34
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.

2 participants