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

In InstrumentDefinitionParser: use of atoi and boost::lexical_cast consistent #10270

Closed
1 task done
Anders-Markvardsen opened this issue May 8, 2014 · 2 comments
Closed
1 task done
Assignees
Labels
Framework Issues and pull requests related to components in the Framework Low Priority Things that you don't ever want to be done. Maintenance Unassigned issues to be addressed in the next maintenance period.

Comments

@Anders-Markvardsen
Copy link
Member

This issue was originally TRAC 9427

This ticket is blocked by :

Originally atoi and atof used to convert string to int and double.

At recently boost::lexical_cast used, which is generally recommended over using atoi and atof, although as was found it http://trac.mantidproject.org/mantid/ticket/9406 boost::lexical_cast does not strip for spaces.

This ticket is proposed to translate all atoi and atof to boost::lexical_cast.

In doing this it has to be checked that the performance of loading IDF does not increase (or only increase very little).

One way of implementing this is to add a private method getStripedAttValue() which returns a string that can be passed to boost::lexical_cast


Keywords: Maintenance

@Anders-Markvardsen
Copy link
Member Author

@FedeMPouzols (2014-11-27T17:19:42):
Use boost::lexical_cast rather than atoi/atof, re http://trac.mantidproject.org/mantid/ticket/9427

5d761ec


Federico M Pouzols (2014-11-28T12:04:34):
I tried replacing the remaining atoi/atof by nested calls to String::strip then boost::lexical_cast to make the InstrumentDefinitionParser consistent in this regard. But I had to step back as this definitely creates issues with tests. Approximation differences between the two alternatives (atoi/f and lexical_cast) make 16 tests fail: http://builds.mantidproject.org/job/develop_incremental/2755/testReport/. For those it should be carefully checked whether it is safe to just increase the error epsilon in the approximate comparisons of results (or add an epsilon, as some tests are doing an exact comparison). This issue occurs on all platforms, and seems to be caused by a difference in precision between atof and stringstream operators.

More in general, there are still quite a few atoi/atof calls in the Mantid code, though not so many, roughly 60 atoi and 115 atof. These are very spread through different components though. There are also a few tens of strtod/strtol/strtof. Maybe a lowish priority maintenance ticket could be created for this. Many issues in the tests can be expected though.


@martyngigg (2014-11-28T12:17:28):
I agree this should definitely be moved to back as maintenance issue as it's too close to release time to be making low-level changes like this and this was more of a clean up than fixing a known issue.

@Anders-Markvardsen Anders-Markvardsen added Low Priority Things that you don't ever want to be done. Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period. labels Jun 3, 2015
@FedeMPouzols
Copy link
Contributor

Archiving this, as many tests turned out to be very sensitive to numerical differences between lexical_cast and atoi/atof.
Low benefit given the amount of higher priority issues.
New geometry being planned which would deprecate this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework Low Priority Things that you don't ever want to be done. Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

No branches or pull requests

2 participants