sandialabs / SpecUtils

A library for opening, manipulating, and exporting gamma spectral files
GNU Lesser General Public License v2.1
27 stars 9 forks source link

radiacode app sometimes mis-identifies the device #20

Closed ckuethe closed 10 months ago

ckuethe commented 10 months ago

This diff adds a check based on the serial number; if a discrepancy is found the instrument_model_ field is patched to the assumed model. As best I can tell, the rest of the spectrum output is correct.

Example files for test suite sent separately.

ckuethe commented 10 months ago

Screenshot_2024-01-04_22-45-46

Using one of the test files and locally compiled interspec, we can see that the warning is propagated up. The fix-up was performed correctly, but/and this does not seem like a thing that should just happen silently.

ckuethe commented 10 months ago

In general, I prefer it when a program tells me "hey, I found a weird thing in your data file. It's not a big problem, but you should probably figure out where that came from." I've got a number of other unrelated apps that do the same thing. They have consistency checks and will let you know if the some part of the input seems a little odd, but they'll try their best to process the input in the absence of a critical error. Silently papering these things over feels a little icky to me.

I've filed a bug report so hopefully the issue will be corrected soon. There may be a second bug, or it's different from what I think it is, since I have examples of an RC-102 being reported as both a 101 and a 103 and I've never had a 101.

If instrument_id_.substr(2,4) fails there are bigger problems. I'm now using a regex to check that the serial number format is correct. If it doesn't match RC-[0-9]{3}-[0-9]{6}, something has gone very wrong and I no longer trust it. I'll update the regex if we see new devices in the wild.

wcjohns commented 10 months ago

That was a good idea to pester developers with the warning when there is a mismatch, but not necessarily bother users. I guess I had assumed the model identification from the serial number should always be correct, is this the case? If so, then yes, lets not bother users (like you have it).

Looking at things a second time, should the line:

if ( instrument_id_.find( model_from_sn ) == string::npos)

maybe instead be

if ( instrument_model_.find( model_from_sn ) == string::npos)

?

ckuethe commented 10 months ago

Yes, good catch.

ckuethe commented 10 months ago

Using the code as of fe7ce21 works as I intend:

this has an invalid serial number (I deleted the last digit) and the device identification isn't fixed up Screenshot_2024-01-05_22-57-45

This has a valid serial number so the fixup code runs. Screenshot_2024-01-05_22-57-19

wcjohns commented 10 months ago

Thanks for fixing this!