Skip to content

Reland Typeassert, with convert on construct#10

Merged
YingboMa merged 4 commits into
YingboMa:masterfrom
chriselrod:typeassert
Jul 13, 2023
Merged

Reland Typeassert, with convert on construct#10
YingboMa merged 4 commits into
YingboMa:masterfrom
chriselrod:typeassert

Conversation

@chriselrod
Copy link
Copy Markdown
Collaborator

I added two tests that fail on Unityper master, and should exemplify the sort of problem that caused the original typeassert commit to be reverted.

@chriselrod chriselrod requested a review from YingboMa July 13, 2023 13:55
@chriselrod
Copy link
Copy Markdown
Collaborator Author

chriselrod commented Jul 13, 2023

Note that three tests were changed, updated to the form

@test c.k === convert(Complex{Real}, 10 + 10im)

This is because the Complex{Int}s stored into the Complex{Real} fields were converted to Complex{Real}.

We have

julia> 1 + 2im isa Complex{Real}
false

so naturally, we shouldn't actually be able to store a Complex{Int} into such a field without conversion.

The goal is to emulate Julia's types more closely (principle of least surprise?).

julia> struct MyStruct
           x::Complex{Real}
       end

julia> MyStruct(1 + 2im).x === 1 + 2im
false

julia> MyStruct(1 + 2im).x === convert(Complex{Real}, 1 + 2im)
true

With this PR, Unityper should start to mirror this behavior.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 13, 2023

Codecov Report

Merging #10 (00a2faa) into master (bcaad77) will increase coverage by 0.05%.
The diff coverage is 87.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   94.40%   94.45%   +0.05%     
==========================================
  Files           3        3              
  Lines         393      397       +4     
==========================================
+ Hits          371      375       +4     
  Misses         22       22              
Impacted Files Coverage Δ
src/compactify.jl 95.16% <87.50%> (+0.05%) ⬆️
@YingboMa
Copy link
Copy Markdown
Owner

Umm, somehow MTK tests aren't running on Julia 1.6. I guess it's still OK to merge since ModelingToolkit.jl//1 passes.

@YingboMa YingboMa merged commit 7b62a03 into YingboMa:master Jul 13, 2023
@chriselrod chriselrod deleted the typeassert branch July 13, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants