Log averaging muon analysis

[AnthonyLim23]

Within the muon analysis code the time averaging was incorrect. It has been altered to use the timeAverageValue function and if the running log is present then it is filtered accordingly.

To test: If the new time averaging is correct

1. Select: Interfaces -> Muon ->Muon Analysis and a new window should open.

2. Load some muon data (e.g. HIFI108376.nxs).

3. Click the Data Analysis tab.

4. Under functions right click and select "add function". In the new box select Background then LinearBackground.

5. Click the Fit button and select Fit from the options.

6. Click the Results Table tab.

7. Select the values that you want to investigate (I have found ICP_SYS_TD to be good).

8. Click Create Table.

9. To create the comparison data right click on MuonAnalysis in the workspaces and select Sample Logs...

10. A window should appear. Select the value that is being investigated.

11. The Time Avg: box should contain the same number as the results table.

12. It is worth checking that the results are different between Status + Period and None. The results should match Status + Period (which is the same as Status) if the running log is present.

Fixes #18061.

Release notes: see here

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.

[martyngigg]

The general form of the changes here is good. The variable names are nice and it is readable.

I think we're possibly over commenting a little bit and when comments are added they should be on the lines preceding the code and not at the end or this leads to logs of whitespace and oddly formatted lines. For example it's not necessary to comment on every line about what is intended. The comments will become out of date and we want readers to focus on the code.

[AnthonyLim23]

I will edit the comments this afternoon and recommit it.

[martyngigg]

We should always initialize variables to some known value. This is not guaranteed to be a nullptr so we should make it one:

const TimeSeriesProperty<bool> *runningLog(nullptr);

[martyngigg]

I would suggest making this a static string constant at the top of the file to avoid repeated evaluations: Placing

const std::string RUNNING_LOG_NAME = "running"

in an anonymous namespace at the top of the file will achieve this.

[AnthonyLim23]

Do you mean like this:

namespace{
const std::string running = "running";
namespace MantidQt {
....code
}//close namespace 

[martyngigg]

Nearly, except the anonymous namespace should be closed straight after the string is defined:

namespace{
const std::string RUNNING_LOG_NAME = "running";
} // end anonymous

I have upper cased the variable name to follow the convention of static constants being defined this way.

[martyngigg]

We can make this const

[martyngigg]

@AnthonyLim23 I've added a few other minor comments but the method is sound.

[AnthonyLim23]

thank you. I will implement them this afternoon

[AnthonyLim23]

updated, that should be all of the changes you suggested. It still works for me.

[martyngigg]

Sorry to be picky but can we have a new line between the #include & namespace and also between the closing } and the start of namespace MantidQt.

Also, please use uppercase letters for static constants - I would suggest renaming running to RUNNING_LOG_NAME.

[martyngigg]

I know this isn't something you have changed but I would suggest adding a & qualifier to the for loop. It's only copying a pointer but I think it's good practice to avoid copies where possible:

for (const auto &prop : logData)

[AnthonyLim23]

Updated the code

[martyngigg]

The code changes look good now.

I've tested the functionality and the results table is now consistent with applying the necessary running/status filters.

Good to go.