Handle invalid pixel IDs in LoadEventNexus #19116

Merged
merged 6 commits into from Mar 29, 2017

Conversation

Projects
None yet
4 participants
@gemmaguest
Contributor

gemmaguest commented Mar 13, 2017

This fixes a debug assertion caused by invalid pixel IDs in an event file.

To test:

Run LoadEventNexusTest in Debug mode. Previously this caused a debug assertion. It should now be fixed.

Fixes #19103 .

Does not need to be in the release notes.


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.

gemmaguest added some commits Mar 10, 2017

Re #19103 Add bounds checking for pixelID_to_wi_vector access
We could just ignore invalid pixels, but given that the range of pixels being
used should now be correct this would indicates a coding error, so I've made
it throw instead.

@PranavBahuguna PranavBahuguna self-assigned this Mar 14, 2017

@@ -242,5 +242,20 @@ void ProcessBankData::run() { // override {
#endif
} // END-OF-RUN()
+// Get the workspace index for a given pixel ID. Throws if the pixel ID is
+// not in the expected range.
+size_t ProcessBankData::getWorkspaceIndexFromPixelID(const detid_t pixID) {

This comment has been minimized.

@PranavBahuguna

PranavBahuguna Mar 15, 2017

Contributor

You should add the '@param' and '@return' tags to the method description for parameters and return respectively.

@PranavBahuguna

PranavBahuguna Mar 15, 2017

Contributor

You should add the '@param' and '@return' tags to the method description for parameters and return respectively.

@PranavBahuguna

This comment has been minimized.

Show comment
Hide comment
@PranavBahuguna

PranavBahuguna Mar 15, 2017

Contributor

Code looks good and tests pass as expected. All that needs to be done is to address the line comment I made and add relevant labels and milestone to this PR.

Contributor

PranavBahuguna commented Mar 15, 2017

Code looks good and tests pass as expected. All that needs to be done is to address the line comment I made and add relevant labels and milestone to this PR.

@peterfpeterson

This comment has been minimized.

Show comment
Hide comment
@peterfpeterson

peterfpeterson Mar 15, 2017

Member

I tried this out on SNAP_36333 (the IDF says they have a pixel 0) and it appears to work. I need to search more for files that actually have counts in pixel 0 though.

Member

peterfpeterson commented Mar 15, 2017

I tried this out on SNAP_36333 (the IDF says they have a pixel 0) and it appears to work. I need to search more for files that actually have counts in pixel 0 though.

@peterfpeterson

This comment has been minimized.

Show comment
Hide comment
@peterfpeterson

peterfpeterson Mar 15, 2017

Member

There are only a couple of instruments that have pixel 0, and it always appears to occur in a dead area of the detector. I think this can be merged without issue for SNS.

Member

peterfpeterson commented Mar 15, 2017

There are only a couple of instruments that have pixel 0, and it always appears to occur in a dead area of the detector. I think this can be merged without issue for SNS.

@PranavBahuguna

This comment has been minimized.

Show comment
Hide comment
Contributor

PranavBahuguna commented Mar 22, 2017

:shipit:

@ianbush ianbush self-assigned this Mar 29, 2017

@ianbush ianbush merged commit 4fb0cd3 into master Mar 29, 2017

9 checks passed

ClangFormat Jenkins build pull_requests-clang-format 12060 has succeeded
Details
Doxygen Jenkins build pull_requests-doxygen 11530 has succeeded
Details
Flake8 Jenkins build pull_requests-flake8 2823 has succeeded
Details
OSX Jenkins build pull_requests-osx 12641 has succeeded
Details
RHEL7 + System Tests Jenkins build pull_requests-rhel7 12562 has succeeded
Details
Ubuntu + Doc Tests Jenkins build pull_requests-ubuntu 13161 has succeeded
Details
Ubuntu Python 3 Jenkins build pull_requests-ubuntu-python3 713 has succeeded
Details
Windows Jenkins build pull_requests-win7 13428 has succeeded
Details
cppcheck Jenkins build pull_requests-cppcheck 13111 has succeeded
Details

@ianbush ianbush deleted the 19103_invalid_pixelID_in_LoadEventNexus branch Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment