Add a help string to Instrument parameters class

[Alex Buts]

According to discussion in developer meeting:

The change would include:

1) Creating a child for Geometry::Parameter class which adds short and long help strings to the class with appropriate accessors/mutators.

2) Modifying LoadIDF (IDFParser) to instantiate the child and populate appropriate properties if <description_short="something"/> and <description_full="more"/> fields are found in an IDF.

3) Exposing these properties to python (accessors).

[Alex Buts]

Re #11508 Initial commit and test project

Changeset: e430e528edc3ba667b9753727dee963b9f6a0f02

[Alex Buts]

Re #11508 Initial rumbling

minor changes to identify change in instrument size if description is added to a parameter

Changeset: 009c89710032eed420b84abf3b52bbb145417051

[Alex Buts]

Re #11508 Adding description to some parameters and param factory

Changeset: 4d472074f002eae259dba37e39c71bf4b75f22c7

[Alex Buts]

Re #11508 finished adding description to all ParameterMap methods

Changeset: 24e7116465f7518f9aae7ff487680a4ed18bf5a6

[Alex Buts]

Re #11508 attempts to fix unit test (unsuccessful yet)

Changeset: 7c24a6d64ea2289a3e435fd0ed09cf10210966fa

[Alex Buts]

Re #11508 fixed old unit tests

Changeset: 8532933f5127abf4bf1e76b32071a4bd0e5e85f5

[Alex Buts]

Re #11508 Unit test for help string

Changeset: 11e742b484d848b41771ba16ca02b24ab41dc1da

[Alex Buts]

Re #11508 Started to modify InstrumenDefinitionParser

to understand description

Changeset: 3d487c1a12415cdef20332cf59c74513605c08f0

[Alex Buts]

Re #11508 redefined tooltip as all before .

and added processing to IDF processing algorithm.

Changeset: 837dcc1314ec5964b44f43a1b17f71438f3a01c1

[Alex Buts]

Re #11508 Looks like can get a parameter description

and unit test for accessor.

Changeset: b1d9aef0c992d3d3f86daf313c43d7d5d7308e67

[Alex Buts]

Re #11508 Help string interface on component

(not yet tested)

Changeset: 671ca69e9da66340ab4c79b0c95273beeada8562

[Alex Buts]

Re #11508 Exported accessors and mutators to python

and it works.

Changeset: 83602c7880bc4be9fb54ea982d720d01588b3153

[Alex Buts]

Re #11508 Doxygen and sample Parameter change

Changeset: 68e13ead9ff5d8d9c900c15e2fd70580ed3fd8a0

[Alex Buts]

Re #11508 One more doxygen error

Changeset: 09b749f45661e73e1205f63f89ddbf7f0b4948a6

[Alex Buts]

Re #11508 fixing xml schema and system tests

Changeset: 4238d8307467123da4116c831afe40ec49a87393

[Alex Buts]

Re #11508 fixing IDF schema

Changeset: 3a68ae938839fb9bba50bab89bc7750d060bf19d

[Alex Buts]

Re #11508 Attempt to identify the reason for failure on MAC

Changeset: 91b4d6e441a9f34a1688cf671625b12cd96b8925

[Alex Buts]

Re #11508 Further chasing MAC compilation problems

Changeset: ebaee2020e7fe2c22a58eef6115dd35bca2c8c3d

[Alex Buts]

Re #11508 fixing cppcheck (and missing description for couple of par)

Changeset: 467e12a566297310617194c36e931ab0b77282d1

[Alex Buts]

Re #11508 Fake changes to kick disappeared MAC test

Changeset: 9f160e0d53f6150f92e9277125240db1940f2297

[Alex Buts]

Re #11508 Trying to get proper build on OSX

Changeset: a36bd2aaa86db9586c314c799855f539ccf4bf27

[Alex Buts]

Re #11508 Nailing down OSX failure.

Completely remove offending includes from test file

Changeset: 4574276cfa97b71407fa66057fcbe6a22c6970a8

[Alex Buts]

Re #11508 Chasing problem with OSX

and a bit more unit testing on the way.

Changeset: 62264196b3f168f91d4ffd95b93938d906245e74

[Alex Buts]

It is fixed as pr ​https://github.com/mantidproject/mantid/pull/621

The changes are simple and I think, relatively small.

I have investigated the memory allocation issue, and when empty string on windows in debug mode takes 40 bytes, a release mode with parameters having empty help string for LET calibrated (~90K detectors) make no difference to memory. This is why point 1) has been simplified and I have just added empty description string to parameters base class.

All new features are unit tested.

Otherwise it does what I wanted it to do -- MARI_Definition and MARI_Parameter string have a test description string added to one of parameters. This string becomes accessible from Python Instrument and user can add a description to an existing parameter from python too -- the benefit of simplifying 1).

Test by code review and common sense -- it is core changes after all.

[Alex Buts]

Re #11508 Addressing the reviewer comments

1) Tooltip name changed to short description

2) Why simply not use m_description = source? because for strings m_description = source compiles into something like if !(&rhs == this)

lhs.assign(rhs)

3) The names of these methods don't properly reflect what they do. For example, Component::getDescription would imply that the description is about the component, not the parameter...

The name means exactly this. Component::getDescription provides the description for a parametrized component. By current agreement (at least enforced by unit tests), a parametrized component would have a parameter with the same name. If it does not, empty description will be returned. If one sets description for a component, a parameter with component name will be created if not already present. I do not need this feature but decided that it would be nice to have as implementation is really trivial, behaves similarly to all other stuff in components and may be useful to somebody.

4) From my perspective, would these not be a better set of methods: getParameterDescription, getShortParameterDescription,setParameterDescription...

Done. Except I've used getParamDescription etc instead of getParameterDescription -- it is as descriptive as suggested but saves a bit of typing. setParamDescription is not implemented. It is not entirely trivial and I do not need it so let's leave it to somebody who(if) actually needs it.

5) I think the description would be more sensible as simply the value of the description element rather than an attribute, i.e....

I make it the same as 12 other (parameters? attributes? components?) parts of the Instrument tree are. Though doing it the suggested way would be better, all other parts are implemented this way. Introducing new entities would be a bit too much for such a small change to the instrument tree. But it its really desired we may create a separate ticket to do such change with this and number of other instrument components. I only suspect that the gain is not worth the efforts, though would be nice.

I think the changes made to the ticket address all remarks.

Changeset: 4f6401612d2b85c07a339c5d74a028c54b143fc2

[Alex Buts]

Re #11508 Pylint warnings

Changeset: 80f37dc4f51e34e3562b638d9442d4c171d11328

[Andrei Savici]

Simple pylint fixes. Refs #11508

Changeset: ec2167e031db7dfe53fee4e8b3f20222a1f1b892

[Andrei Savici]

If you have 3000 trivial pylint warnings you don't see what's important

[Martyn Gigg]

I'm not going to have time to look at this before I leave.

[Andrei Savici]

Trying to fix systemtests. Refs #11508

Changeset: 1eb54884d90926ac380fa3fe4a5e5bdd4051218c

[Roman Tolchenov]

@abuts Could you merge master and resolve the conflicts please?

[Alex Buts]

Re #11508 resolving conflicts with master

Merge branch 'master' into 11508_ParamWithHelpString

Conflicts:

Code/Mantid/Framework/Geometry/src/Instrument/Component.cpp Code/Mantid/Framework/PythonInterface/mantid/geometry/src/Exports/Component.cpp

Changeset: 74d51475a0911ef9437b24eeed5fd65dad30ea9f

[Alex Buts]

Re #11508 Fixed merge error.

Changeset: 85b034c2d98d03a50d72a9a270a10871c1788f3d

[Martyn Gigg]

@mantid-roman The Windows doc tests failures are spurious an can be ignored for these tests. @abuts It looks like there are still merge conflicts

[Alex Buts]

Re #11508 merging master to fix doctest as changes are due to the bug

fixed there.

Merge branch 'master' into 11508_ParamWithHelpString

Conflicts:

Code/Mantid/Framework/PythonInterface/mantid/geometry/src/Exports/Component.cpp

Changeset: 4f2b374c2ebff511165056968b9c6b98e3f5e4d8

[Alex Buts]

Re #11508 Initial commit and test project

Changeset: e430e528edc3ba667b9753727dee963b9f6a0f02

[Alex Buts]

Re #11508 Initial rumbling

minor changes to identify change in instrument size if description is added to a parameter

Changeset: 009c89710032eed420b84abf3b52bbb145417051

[Alex Buts]

Re #11508 Adding description to some parameters and param factory

Changeset: 4d472074f002eae259dba37e39c71bf4b75f22c7

[Alex Buts]

Re #11508 finished adding description to all ParameterMap methods

Changeset: 24e7116465f7518f9aae7ff487680a4ed18bf5a6

[Alex Buts]

Re #11508 attempts to fix unit test (unsuccessful yet)

Changeset: 7c24a6d64ea2289a3e435fd0ed09cf10210966fa

[Alex Buts]

Re #11508 fixed old unit tests

Changeset: 8532933f5127abf4bf1e76b32071a4bd0e5e85f5

[Alex Buts]

Re #11508 Unit test for help string

Changeset: 11e742b484d848b41771ba16ca02b24ab41dc1da

[Alex Buts]

Re #11508 Started to modify InstrumenDefinitionParser

to understand description

Changeset: 3d487c1a12415cdef20332cf59c74513605c08f0

[Alex Buts]

Re #11508 redefined tooltip as all before .

and added processing to IDF processing algorithm.

Changeset: 837dcc1314ec5964b44f43a1b17f71438f3a01c1

[Alex Buts]

Re #11508 Looks like can get a parameter description

and unit test for accessor.

Changeset: b1d9aef0c992d3d3f86daf313c43d7d5d7308e67

[Alex Buts]

Re #11508 Help string interface on component

(not yet tested)

Changeset: 671ca69e9da66340ab4c79b0c95273beeada8562

[Alex Buts]

Re #11508 Exported accessors and mutators to python

and it works.

Changeset: 83602c7880bc4be9fb54ea982d720d01588b3153

[Alex Buts]

Re #11508 Doxygen and sample Parameter change

Changeset: 68e13ead9ff5d8d9c900c15e2fd70580ed3fd8a0

[Alex Buts]

Re #11508 One more doxygen error

Changeset: 09b749f45661e73e1205f63f89ddbf7f0b4948a6

[Alex Buts]

Re #11508 fixing xml schema and system tests

Changeset: 4238d8307467123da4116c831afe40ec49a87393

[Alex Buts]

Re #11508 fixing IDF schema

Changeset: 3a68ae938839fb9bba50bab89bc7750d060bf19d

[Alex Buts]

Re #11508 Attempt to identify the reason for failure on MAC

Changeset: 91b4d6e441a9f34a1688cf671625b12cd96b8925

[Alex Buts]

Re #11508 Further chasing MAC compilation problems

Changeset: ebaee2020e7fe2c22a58eef6115dd35bca2c8c3d

[Alex Buts]

Re #11508 fixing cppcheck (and missing description for couple of par)

Changeset: 467e12a566297310617194c36e931ab0b77282d1

[Alex Buts]

Re #11508 Fake changes to kick disappeared MAC test

Changeset: 9f160e0d53f6150f92e9277125240db1940f2297

[Alex Buts]

Re #11508 Trying to get proper build on OSX

Changeset: a36bd2aaa86db9586c314c799855f539ccf4bf27

[Alex Buts]

Re #11508 Nailing down OSX failure.

Completely remove offending includes from test file

Changeset: 4574276cfa97b71407fa66057fcbe6a22c6970a8

[Alex Buts]

Re #11508 Chasing problem with OSX

and a bit more unit testing on the way.

Changeset: 62264196b3f168f91d4ffd95b93938d906245e74

[Alex Buts]

Re #11508 Addressing the reviewer comments

1) Tooltip name changed to short description

2) Why simply not use m_description = source? because for strings m_description = source compiles into something like if !(&rhs == this)

lhs.assign(rhs)

3) The names of these methods don't properly reflect what they do. For example, Component::getDescription would imply that the description is about the component, not the parameter...

The name means exactly this. Component::getDescription provides the description for a parametrized component. By current agreement (at least enforced by unit tests), a parametrized component would have a parameter with the same name. If it does not, empty description will be returned. If one sets description for a component, a parameter with component name will be created if not already present. I do not need this feature but decided that it would be nice to have as implementation is really trivial, behaves similarly to all other stuff in components and may be useful to somebody.

4) From my perspective, would these not be a better set of methods: getParameterDescription, getShortParameterDescription,setParameterDescription...

Done. Except I've used getParamDescription etc instead of getParameterDescription -- it is as descriptive as suggested but saves a bit of typing. setParamDescription is not implemented. It is not entirely trivial and I do not need it so let's leave it to somebody who(if) actually needs it.

5) I think the description would be more sensible as simply the value of the description element rather than an attribute, i.e....

I make it the same as 12 other (parameters? attributes? components?) parts of the Instrument tree are. Though doing it the suggested way would be better, all other parts are implemented this way. Introducing new entities would be a bit too much for such a small change to the instrument tree. But it its really desired we may create a separate ticket to do such change with this and number of other instrument components. I only suspect that the gain is not worth the efforts, though would be nice.

I think the changes made to the ticket address all remarks.

Changeset: 4f6401612d2b85c07a339c5d74a028c54b143fc2

[Alex Buts]

Re #11508 Pylint warnings

Changeset: 80f37dc4f51e34e3562b638d9442d4c171d11328

[Andrei Savici]

Simple pylint fixes. Refs #11508

Changeset: ec2167e031db7dfe53fee4e8b3f20222a1f1b892

[Andrei Savici]

Trying to fix systemtests. Refs #11508

Changeset: 1eb54884d90926ac380fa3fe4a5e5bdd4051218c

[Alex Buts]

Re #11508 resolving conflicts with master

Merge branch 'master' into 11508_ParamWithHelpString

Conflicts:

Code/Mantid/Framework/Geometry/src/Instrument/Component.cpp Code/Mantid/Framework/PythonInterface/mantid/geometry/src/Exports/Component.cpp

Changeset: 74d51475a0911ef9437b24eeed5fd65dad30ea9f

[Alex Buts]

Re #11508 Fixed merge error.

Changeset: 85b034c2d98d03a50d72a9a270a10871c1788f3d

[Alex Buts]

Re #11508 merging master to fix doctest as changes are due to the bug

fixed there.

Merge branch 'master' into 11508_ParamWithHelpString

Conflicts:

Code/Mantid/Framework/PythonInterface/mantid/geometry/src/Exports/Component.cpp

Changeset: 4f2b374c2ebff511165056968b9c6b98e3f5e4d8

[Roman Tolchenov]

Merge pull request #621 from mantidproject/11508_ParamWithHelpString

Help strings in parameter classes

Full changeset: ​a6afa85b50312aacc41afe0c8cbe359b1918e8ee

[Stuart Campbell]

This ticket has been transferred to github issue ​12346