Fix an issue in TimeSeriesProperty

[raquelalvarezbanos]

Description of work.

To test:

• This has to be tested on Windows, debug mode.
• Run the script in the issue description, Mantid shouldn't crash.
• Make sure that the new unit test fails without this fix.
• I could have added a similar check for the property values before this line, but I think it is unnecessary. Make sure you agree with me.

Fixes #18944.

No need to mention this in the release notes, as the bug only happens on Windows in debug mode.

---

Reviewer

Please comment on the following (full description):

Code Review

• Is the code of an acceptable quality?
• Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
• Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
• If there are changes in the release notes then do they describe the changes appropriately?

Functional Tests

• Do changes function as described? Add comments below that describe the tests performed?

• How do the changes handle unexpected situations, e.g. bad input?

• Has the relevant documentation been added/updated?

• Is user-facing documentation written in a user-friendly manner?

• Has developer documentation been updated if required?

• Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

[peterfpeterson]

Shouldn't you use TimeSeriesProperty::size() or TimeSeriesProperty::realSize() and copy the vectors if they aren't equal?

[dtasev]

👍 maybe extend the if (this->m_size != right.m_size) above with an || this->realSize() != right.realSize(), then the check will be applied for both below

[peterfpeterson]

The greatest effect would be had by putting this as an if on line 176.

[dtasev]

@peterfpeterson Could you tell me why an additional if is preferred to extending an existing one?

[raquelalvarezbanos]

Shouldn't they be the same? I've just added the additional because I find it easier to read.

[dtasev]

I've tested without the fix and mantid crashes. After applying the fix there is no crash.
I've ran the unit tests without the fix and the new test fails. After applying the fix it passes successfully.
If @peterfpeterson doesn't have any comments I'm happy to

[peterfpeterson]

add the word else here and I'm happy

[peterfpeterson]

I think a clearer version of the code (with added const below) is:

if (this->realSize() != right.realSize()) {
     return false;
} else {
     const std::vector<DateAndTime> lhsTimes = this->timesAsVector();
      const std::vector<DateAndTime> rhsTimes = right.timesAsVector();
     if (!std::equal(lhsTimes.begin(), lhsTimes.end(), rhsTimes.begin())) {
       return false;
     }

     const std::vector<TYPE> lhsValues = this->valuesAsVector();
     const std::vector<TYPE> rhsValues = right.valuesAsVector();
     if (!std::equal(lhsValues.begin(), lhsValues.end(), rhsValues.begin())) {
       return false;
     }
}

return true;

I don't think the const buys anything since those functions return copies of the vectors. Without checking against a compiler, you can probably declare the various variables as const auto as well, but I don't think it really adds.

[martyngigg]

Tests are the usual unrelated failures.