search

JuliaDynamics / Attractors.jl

Find attractors (and their basins) of dynamical systems. Perform global continuation. Study global stability (a.k.a. non-local, or resilience). Also tipping points functionality.
MIT License
32 stars 6 forks source link

Function `_rescale_to_01` is incorrect #55

Closed KalelR closed 1 year ago

KalelR commented 1 year ago

Hey

I just looked at the _rescale_to_01 function, which we use to rescale each feature dimension onto the [0,1] interval, and noticed it is clearly incorrect.

Currently, it's 

function _rescale_to_01(features::AbstractStateSpaceSet)
    mini, maxi = minmaxima(features)
    return map(f -> f .* (maxi .- mini) .+ mini, features)
end

This does not rescale features to [0,1].

Instead, it should be 

function _rescale_to_01(features::AbstractStateSpaceSet)
    mini, maxi = minmaxima(features)
    return map(f -> (f .- mini) ./ (maxi .- mini), features)
end

When I first wrote it, it was correct, I'm pretty sure. Somewhere in the middle of the refactoring we got this wrong. It might very well nott affect results, but we should correct it to do what it should.

I'm writing an issue because I implemented this and the dummy bistable test in grouping_continuation.jl got an error. I didn't have time to understand why this happens.. but I saw that this rescaling of features leads to small floating point errors (e.g. the rescaled features being 0.49999999...2 instead of 0.5). Maybe the matching procedure does not match them because of the errors, thus creating new attractors? Or is the rescaling applied to the global pool (which would rescale the features a certain way) and then also applied to a local pool, for only attractor (whihc would rescale in a different way)?

Datseris commented 1 year ago

Well sure why not do the PR that fixes the function?

Datseris commented 1 year ago

seems to me you are correct and there isn't much debate to be had here :D

KalelR commented 1 year ago

I can, but that test will fail. If you have an idea why, I could already fix it in the PR.

Datseris commented 1 year ago

i don't understand which test you are referring to.

Datseris commented 1 year ago

bump

KalelR commented 1 year ago

The dummy bistable map in grouping_continuation.jl tests fails with this change. It apparently leads to floating point errors that somehow screw up the matching. The reason for this is not obvious to me, but I thought could be for you. If not, I can take a look at this sometime in the next days :)

Datseris commented 1 year ago

Can you open a PR that does this, and then I look at the failing test?