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

Use missing instead of nothing for dynamic values #232

Merged
merged 8 commits into from
Jan 10, 2022
Merged

Use missing instead of nothing for dynamic values #232

merged 8 commits into from
Jan 10, 2022

Conversation

Tokazama
Copy link
Member

No description provided.

@Tokazama
Copy link
Member Author

For some reason all tests are failing but at different places for different Julia versions. I'll have to do some more poking around.

@chriselrod
Copy link
Collaborator

chriselrod commented Dec 30, 2021

For some reason all tests are failing but at different places for different Julia versions. I'll have to do some more poking around.

This should fix some (but not all) of them: #230

@chriselrod
Copy link
Collaborator

chriselrod commented Dec 31, 2021

Odd that this is getting an ambiguity error on lu for StaticArrays, when master isn't, and that code hasn't been touched in this PR?

I'm not getting that locally, but I am getting a lot of obviously-related failures from maybe_static -> static(::Missing).

@Tokazama
Copy link
Member Author

I assumed that missing would have similar behavior as nothing since they're both singletons, but maybe there's some sort of compiler wizardry going on with nothing I'm not aware of.

…d === for comparisons, and Static.find_first_eq still returns nothing on failure, like Base.findfirst
@chriselrod
Copy link
Collaborator

The lu ambiguities are because it was using an old version of StaticArrays, before:
JuliaArrays/StaticArrays.jl@7dc6499

We could add a test/Project.toml to restrict the version it tests with.

@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #232 (452903c) into master (687fdfb) will increase coverage by 0.03%.
The diff coverage is 92.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   88.97%   89.01%   +0.03%     
==========================================
  Files          11       11              
  Lines        1751     1730      -21     
==========================================
- Hits         1558     1540      -18     
+ Misses        193      190       -3     
Impacted Files Coverage Δ
src/size.jl 80.48% <50.00%> (ø)
src/array_index.jl 97.36% <66.66%> (-0.82%) ⬇️
src/stridelayout.jl 87.36% <77.08%> (ø)
src/ranges.jl 91.59% <83.33%> (-0.08%) ⬇️
src/ArrayInterface.jl 90.11% <99.27%> (+3.05%) ⬆️
src/axes.jl 92.85% <100.00%> (ø)
src/dimensions.jl 97.70% <100.00%> (ø)
src/indexing.jl 85.85% <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 687fdfb...452903c. Read the comment docs.

@chriselrod
Copy link
Collaborator

Doc tests fail, but should pass once the pkg server updates and has Static 0.5 available.
So this should be good to go.

@Tokazama
Copy link
Member Author

Since this is breaking, do we want to wait to register this until we manage other breaking changes like moving BandedMatrixIndex out

@chriselrod
Copy link
Collaborator

Since this is breaking, do we want to wait to register this until we manage other breaking changes like moving BandedMatrixIndex out

Sure, no rush.

@Tokazama
Copy link
Member Author

Tokazama commented Jan 7, 2022

I just moved the banded matrix stuff into requires so that when we move to other packages nothing should change.

@Tokazama Tokazama merged commit ca6cfd1 into master Jan 10, 2022
@Tokazama Tokazama deleted the v3.3 branch January 10, 2022 02:01
@YingboMa
Copy link
Contributor

Is this breaking change really worth to make? The whole SciML ecosystem is still forced on v3 and not able to use new features.

@ChrisRackauckas
Copy link
Member

It's been too long without downstream PRs. Let's revert it today.

@Tokazama
Copy link
Member Author

Is the problem just with getting LoopVectorization updated or do we need to go through the whole ecosystem? I'd be surprised if many packages had to change anything to accommodate this.

ChrisRackauckas added a commit that referenced this pull request Mar 1, 2022
#232 was found to be a huge mistake. For interface checks, you want `nothing` instead of `missing` because you want to handle the cases. The issue with `missing` is that it propagates, `missing + 1 == missing`. We don't want that here, we want clear and precise error messages at the spot where the `nothing` is found if it's not supposed to be there and it's supposed to be handled. That is antithetical to most of its usage here. So this is a strong revert with a breaking update, which should fix downstream libraries (LoopVectorization, OrdinaryDiffEq, etc.) which were never received the proper downstream PRs from the original change anyways.
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.

4 participants