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

type instability fix #132

Merged
merged 12 commits into from
Jan 25, 2023
Merged

type instability fix #132

merged 12 commits into from
Jan 25, 2023

Conversation

daviehh
Copy link
Contributor

@daviehh daviehh commented Jan 23, 2023

Tentative PR for the type instability/runtime dispatch and the resulting long loading time, may fix #128
Maybe it may be handled in a more clean/elegant fashion..

Also, since eltype(<:Number) just returns the type of the number, the line

u_elType = uType <: Number ? uType : eltype(u)

maybe just changed to u_elType = eltype(u). Not sure how robust it can be

@Deltadahl
Copy link
Contributor

Deltadahl commented Jan 23, 2023

Nice one!
Just before I had to leave yesterday I also found the problem and solved it with

max_trust_radius = convert(typeof(max_trust_radius), max(norm(fu), maximum(u) - minimum(u)))

in #131. Sorry for not telling you, but I thought this comment #131 (comment) would trigger a note in #128.
Anyhow, maybe your way to solve it is better, nice work :)

@ChrisRackauckas
Copy link
Member

                     max_trust_radius::Real = 0.0,
                     initial_trust_radius::Real = 0.0,
                     step_threshold::Real = 0.1,
                     shrink_threshold::Real = 0.25,
                     expand_threshold::Real = 0.75,
                     shrink_factor::Real = 0.25,
                     expand_factor::Real = 2.0,

what we really should be doing is change those values to Rational numbers. If that is done, they will convert on-demand when hitting u types, for example 1//2 * 0.33 == 0.165 and 1//2 * 3.3f-2 == 0.0165f0. That makes the default values "type neutral". I wouldn't require that the numbers are rational, I would expect users to give Float64 value (or correctly type-matching values), but since the overwhelming default will be for the user to just use the default values of the algorithm parameters, this would be a safer option. The correct conversions should also then be done when the values are put into the cache object as well, doing a convert(eltype(u),x) on all of them.

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #132 (b902aa3) into master (e1c35f5) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   91.00%   91.28%   +0.28%     
==========================================
  Files           7        7              
  Lines         500      505       +5     
==========================================
+ Hits          455      461       +6     
+ Misses         45       44       -1     
Impacted Files Coverage Δ
src/trustRegion.jl 98.46% <100.00%> (+0.06%) ⬆️
src/utils.jl 66.15% <0.00%> (+1.53%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Deltadahl
Copy link
Contributor

hmm, I made the changes suggested by Chris, but I wasn't allowed to push to @daviehh
(Think its because: "The author of the PR also may need to explicitly allow you to push to their branch." in https://gist.github.com/wtbarnes/56b942641d314522094d312bbaf33a81)

@ChrisRackauckas
Copy link
Member

You can PR to his branch.

@ChrisRackauckas
Copy link
Member

Hmm something happened with the merge ambiguity fix. It's probably just a simple thing with the type parameters.

@Deltadahl
Copy link
Contributor

Deltadahl commented Jan 24, 2023

The problem is that step_threshold and step_size got different types (trustType, suType and uType) see the "arrows" (<-----------------)

    step_threshold::trustType <-----------------
    shrink_threshold::trustType
    expand_threshold::trustType
    shrink_factor::trustType
    expand_factor::trustType
    loss::floatType
    loss_new::floatType
    H::jType
    g::resType
    shrink_counter::Int
    step_size::suType  <-----------------
    u_tmp::tmpType
    fu_new::resType
    make_new_J::Bool
    r::floatType

    function TrustRegionCache{iip}(f::fType, alg::algType, u::uType, fu::resType, p::pType,
                                   uf::ufType, linsolve::L, J::jType,
                                   jac_config::JC, iter::Int,
                                   force_stop::Bool, maxiters::Int, internalnorm::INType,
                                   retcode::SciMLBase.ReturnCode.T, abstol::tolType,
                                   prob::probType, trust_r::trustType,
                                   max_trust_r::trustType, step_threshold::suType, <-----------------
                                   shrink_threshold::trustType, expand_threshold::trustType,
                                   shrink_factor::trustType, expand_factor::trustType,
                                   loss::floatType, loss_new::floatType, H::jType,
                                   g::resType, shrink_counter::Int, step_size::uType, <-----------------
                                   u_tmp::tmpType, fu_new::resType, make_new_J::Bool,

Unfortunately I got no time to fix it right now, but if its not fixed in ~8h I'll have time to do it

src/trustRegion.jl Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 2ab52d6 into SciML:master Jan 25, 2023
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.

package loading time regression
3 participants