Skip to content

Conversation

@ashlaban
Copy link
Contributor

There was an infinite loop on the arm platform when running TMVAMulticlass.root. When compiling with the -ffast-math flag, sometimes a nan would be generated in the GA part of the cut optimisation.

@dpiparo @martinmine Hopefully this resolves your issue. Tried it on the build machine I was given access to and it works there now. We in the TMVA team still need to revisit this part at some point, but for now I think this should be ok.

There was an infinite loop on the arm platform when running
TMVAMulticlass.root. When compiling with the -ffast-math flag,
sometimes a nan would be generated.
@phsft-bot
Copy link

Can one of the admins verify this patch?

@martinmine
Copy link
Contributor

@phsft-bot build!

@martinmine
Copy link
Contributor

Awesome, thanks a lot for taking your time looking into this!

@dpiparo
Copy link
Member

dpiparo commented Mar 20, 2017

Hi @ashlaban : what quantity ends up being a nan? Maybe we can avoid this with a few checks.

@ashlaban
Copy link
Contributor Author

ashlaban commented Mar 20, 2017

@dpiparo The GA optimiser uses toMinimize in ResultsMulticlass::EstimatorFunction to evaluate the fitness. This variable ended up being NaN if effTimesPur was very close to zero because of the assignment toMinimize = 1. / effTimesPur. The already existing check is extended to avoid this situation (an std::isnan does not work since we compile with -ffast-math).

I also verified that 1. / std::numeric_limits<float>::min() does not generate a NaN, so the function should be safe now.

@dpiparo
Copy link
Member

dpiparo commented Mar 20, 2017

Hi @ashlaban,
it's true: std::isnan does not work. For this reason, in ROOT, we have TMath::IsNan :)
I am not sure this solves this very problem though.
In any case I would protect this code with a preprocessor statement in order to activate it on arm only if you agree.

@ashlaban
Copy link
Contributor Author

Hi @dpiparo,

Ah! That is good to know!

I definitely think this solves one of the problems on the arm platform, could we be talking about different issues?

Regarding the preprocessor protect:
The change really should not make a difference with respect to the output regardless of platform. If the score of the particular cut so bad that this fix makes a difference we either:

  • find a better cut value and forget about this one or,
  • this is a solution that would not have been chosen without the patch, but both of these solutions are really really really bad so it doesn't matter which ones we choose.

So since I don't think it will matter either way, feel free to do as you feel best. One point is that the code flow will be more clear, in my mind, without any preprocessor branching.

@dpiparo
Copy link
Member

dpiparo commented Mar 20, 2017

Hi,

if the numerical stability of the algorithm is guaranteed, I am in favour of not cluttering the code with preprocessor statements 👍

@ashlaban
Copy link
Contributor Author

I guarantee it.

@martinmine
Copy link
Contributor

@phsft-bot build!

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