- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 82
Add simulation type preservation tests #1198
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
Conversation
| osol = solve(oprob) | ||
| @test eltype(osol[:X1]) == eltype(osol[:X2]) == typeof(oprob[:X1]) == typeof(oprob[:X2]) == Float64 | ||
| @test eltype(osol.t) == Float64 | ||
| @test typeof(oprob.tspan[1]) == typeof(oprob.tspan[2]) == Int64 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we care about these remaining integers? That seems degenerate since at some point they will have to be converted to floating point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is asking for trouble since converting them earlier seems reasonable and could be implemented at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I really don't care what the type is. However I don't like the idea of theses suddenly jumping around (as it is likely non-intentional and a sign something else might be going on. And if it is intentional it is easy to switch here)
I will ask about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So apparently the current policy is for XProblem to not do any type promotion. I'm inclined to adding a comment to the test about the situation, and if how XProblems work changes we change the test (after confirming that it is intentional). The potential drawback seems small.
But if you want to remove I will do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d still prefer to use problem appropriate types. So use Float32/16/64 for such tests. It just seems like we are testing a degenerate case here that they could reasonably decide to promote at any moment (since ultimately it is getting promoted during solves), and could then break this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even use BigFloat if you want a less common type.
| ssol = solve(sprob, ISSEM()) | ||
| @test eltype(ssol[:X1]) == eltype(ssol[:X2]) == typeof(sprob[:X1]) == typeof(sprob[:X2]) == Float64 | ||
| @test eltype(ssol.t) == Float64 | ||
| @test typeof(sprob.tspan[1]) == typeof(sprob.tspan[2]) == Int64 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about types
| I have removes any checks on whether a time value stored in a problem is an integer. In the cases where integer times are used for creation, we don't perform the check, and just checks that it is promoted to the appropriate time in the solution. | 
Checks that solution values (and time/problem values for good measure) are kept with the input type. Most cases I do not really care about, but if there are sudden changes in behaviours I would like to catch that. Importantly it prevents another case of SciML/ModelingToolkit.jl#3446
Won't pass until SciML/ModelingToolkit.jl#3448 filter though.