Fix bug in LoadNGEM where Min/MaxEventsPerFrame were not respected #38564

RichardWaiteSTFC · 2025-01-06T16:53:05Z

Description of work

Added loop to clear events in frame if the current frame is not good (i.e. number of events outside Min/Max limits set) in preparation for next frame (don't know if that is the best way to do it)..

The issue was here

mantid/Framework/DataHandling/src/LoadNGEM.cpp

Lines 74 to 88 in 22bed21

    
             if (eventCountInFrame >= minEventsReq && eventCountInFrame <= maxEventsReq) { 
        
               // Add number of event counts to workspace. 
        
               frameEventCounts.emplace_back(eventCountInFrame); 
        
               ++goodFrames; 
        
               PARALLEL_FOR_NO_WSP_CHECK() 
        
               // Add events that match parameters to workspace 
        
               for (auto i = 0; i < NUM_OF_SPECTRA; ++i) { 
        
                 if (eventsInFrame[i].getNumberEvents() > 0) { 
        
                   events[i] += eventsInFrame[i]; 
        
                   eventsInFrame[i].clear(); 
        
                 } 
        
               } 
        
             } 
        
           }

If the statement on L74 is false then eventsInFrame doesn't get cleared - such that the sum over all eventsInFrame[i].getNumberEvents() is not equal to eventCountInFrame

Fixes #38558

Report to: draspi

To test:

(1) Follow instructions on the issue
(2) For MinEventsPerFrame=0 and MaxEventsPerFrame=0 the workspace should have no counts (ws.getNumberEvents() = 0)
(3) If MinEventsPerFrame and MaxEventsPerFrame are not set then the resulting figure should look like the one in the issue.

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

In preparation for next frame

The condition ++numWordsSkipped is always true, but it will only get evaluated if !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() is true which will indicate a word being skipped, so this does actually have a function!

cailafinn

Working well, the correct counts are now shown in the output. Just a quick question about one of the changes from the CppCheck suppressions.

cailafinn · 2025-01-08T11:02:49Z

Framework/DataHandling/src/LoadNGEM.cpp

-    } while (!event.generic.check() && !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() && ++numWordsSkipped);
+    } while (!event.generic.check() && !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof());
    if (file.eof()) {
+      ++numWordsSkipped;
      break; // we have either not read an event, or only read part of one
    }


I don't think these are equivalent. The numWordsSkipped variable indicates the number of words (multiples of 4 bytes) that were skipped at the beginning of the file before the algorithm found a valid event. I think this will just mark a skipped word whenever the end of a file is reached.

Yep sorry, I think I'm confused by this
!file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof()
would this terminate the loop if the current position in the file is within SKIP_WROD_SIZE of the end of the file?
i.e. would the do/while loop end before eof?

I've looked at the docs for seekg and am none the wiser! Not had to do much file IO in C++!

So its main check is if it was able to load an event into the EventUnion object of some number of words (4 bytes) using event.generic.check(). If it could do that, it exits the loop. If it couldn't create a valid event, it checks if it can skip 1 word forward file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() and exits the loop if it would be unable to progress because you reached the end of the file. Otherwise, it increments the number of skipped words and then restarts the loop to try to create an event object from the next word. It repeats this until it is able to create an valid event object.

Ah OK I see thanks, so the counter ++numWordsSkipped represents the number of times file.seekg(SKIP_WORD_SIZE, std::ios_base::cur) has been run without reaching the end of the file?

Think my last commit is now equivalent - seems a bit clunky (maybe could be improved) - but also happy to revert and keep the suppression. whatever you prefer!

cailafinn

Change looks good and this is probably easier to read than the original style, just a nitpick (sorry) on the variable naming:

cailafinn · 2025-01-16T08:58:46Z

Framework/DataHandling/src/LoadNGEM.cpp

+    bool is_event_invalid = true;
+    bool is_not_eof_after_skip = true;


These want to be in camelCase as they're C++ variables.

Thanks, sorry too much time with python! Force pushed to fix this

cailafinn

Looks good to me now. Min/Max now properly working as expected.

This is a squashed version of mantidproject#38564 Clear events in frame if not good frame In preparation for next frame Update cpp check suppression The condition ++numWordsSkipped is always true, but it will only get evaluated if !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() is true which will indicate a word being skipped, so this does actually have a function! Update test to assert no events if min=max=0 Add release note Fix cpp check supression

Clear events in frame if not good frame

6748ce5

In preparation for next frame

RichardWaiteSTFC added Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS labels Jan 6, 2025

RichardWaiteSTFC added this to the Release 6.12 milestone Jan 6, 2025

RichardWaiteSTFC marked this pull request as draft January 6, 2025 16:54

RichardWaiteSTFC added 3 commits January 6, 2025 19:20

Update cpp check suppression

a44a433

The condition ++numWordsSkipped is always true, but it will only get evaluated if !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() is true which will indicate a word being skipped, so this does actually have a function!

Update test to assert no events if min=max=0

9f08de9

Add release note

b1612c5

RichardWaiteSTFC marked this pull request as ready for review January 8, 2025 10:01

cailafinn self-assigned this Jan 8, 2025

cailafinn requested changes Jan 8, 2025

View reviewed changes

RichardWaiteSTFC force-pushed the 38558_LoadNGEM_not_respecting_min_max_events branch from af088c8 to 77aae45 Compare January 14, 2025 13:58

cailafinn requested changes Jan 16, 2025

View reviewed changes

Fix cpp check supression

45eeeac

RichardWaiteSTFC force-pushed the 38558_LoadNGEM_not_respecting_min_max_events branch from 77aae45 to 45eeeac Compare January 16, 2025 09:49

cailafinn approved these changes Jan 16, 2025

View reviewed changes

thomashampson self-assigned this Jan 16, 2025

thomashampson merged commit fc0059c into main Jan 16, 2025
10 checks passed

thomashampson deleted the 38558_LoadNGEM_not_respecting_min_max_events branch January 16, 2025 14:06

peterfpeterson mentioned this pull request Jan 16, 2025

Update cppcheck to 2.16.0 - ornlnext #38614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in LoadNGEM where Min/MaxEventsPerFrame were not respected #38564

Fix bug in LoadNGEM where Min/MaxEventsPerFrame were not respected #38564

RichardWaiteSTFC commented Jan 6, 2025 •

edited

Loading

cailafinn left a comment

cailafinn Jan 8, 2025

RichardWaiteSTFC Jan 8, 2025

RichardWaiteSTFC Jan 8, 2025 •

edited

Loading

cailafinn Jan 8, 2025

RichardWaiteSTFC Jan 8, 2025

RichardWaiteSTFC Jan 14, 2025

cailafinn left a comment

cailafinn Jan 16, 2025

RichardWaiteSTFC Jan 16, 2025

cailafinn left a comment

	if (eventCountInFrame >= minEventsReq && eventCountInFrame <= maxEventsReq) {
	// Add number of event counts to workspace.
	frameEventCounts.emplace_back(eventCountInFrame);
	++goodFrames;

	PARALLEL_FOR_NO_WSP_CHECK()
	// Add events that match parameters to workspace
	for (auto i = 0; i < NUM_OF_SPECTRA; ++i) {
	if (eventsInFrame[i].getNumberEvents() > 0) {
	events[i] += eventsInFrame[i];
	eventsInFrame[i].clear();
	}
	}
	}
	}

		bool is_event_invalid = true;
		bool is_not_eof_after_skip = true;

Fix bug in LoadNGEM where Min/MaxEventsPerFrame were not respected #38564

Fix bug in LoadNGEM where Min/MaxEventsPerFrame were not respected #38564

Conversation

RichardWaiteSTFC commented Jan 6, 2025 • edited Loading

Description of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

cailafinn left a comment

Choose a reason for hiding this comment

cailafinn Jan 8, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 8, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

cailafinn Jan 8, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 8, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 14, 2025

Choose a reason for hiding this comment

cailafinn left a comment

Choose a reason for hiding this comment

cailafinn Jan 16, 2025

Choose a reason for hiding this comment

RichardWaiteSTFC Jan 16, 2025

Choose a reason for hiding this comment

cailafinn left a comment

Choose a reason for hiding this comment

RichardWaiteSTFC commented Jan 6, 2025 •

edited

Loading

RichardWaiteSTFC Jan 8, 2025 •

edited

Loading