Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Radionuclide code improvements #1026

Merged
merged 15 commits into from
May 11, 2022

Conversation

KrisThielemans
Copy link
Collaborator

I created a test for Radionuclide code and then had to fix various things to get it to work/understand.

The test fails locally because the JSON file has PET and nucmed but ImagingModality uses PT and NM as strings. We could fix this in the code or the JSON file. Not sure what the best option is. @danieldeidda ?

In addition, I think this will fail on Github Actions etc as the test code is run before install, so it won't find the JSON files.

@KrisThielemans
Copy link
Collaborator Author

It seems that I've been a bit too enthusiastic with adding error statements as we now have a lot of failures with

ERROR: RadioNuclideDB::get_radionuclide: unknown modality

even for the tests where RadioNuclide isn't used. Not sure where/why we're calling this but apparently more than I hoped.

Appveyor (i.e. no JSON) is failing with https://ci.appveyor.com/project/KrisThielemans/stir/builds/43273181/job/r0j359avdtqrl8iw#L133

c:\projects\stir\src\buildblock\radionuclidedb.cxx(196): error C4716: 'stir::RadionuclideDB::get_radionuclide_from_json': must return a value [C:\projects\stir\build\src\buildblock\buildblock.vcxproj]

and also has warnings about unused variables.

- better handling of defaults in RadionuclideDB ("default" or "")
- more checks/info when JSON support is disabled
- more information when JSON parsing failed
- more doxygen
this will likely fail because of 2 reasons:
- modality keyword problems
- when test code is run before install, it won't find the JSON files
changed keV and branching_ratio
ctest can be run before installation, so we need to set the STIR_CONFIG_DIR
environment variable to where the JSON files are in the source directory
Calling error() was too drastic as most files will not have the
necessary info. Instead use warning() and return default-constructed
Radionuclide (i.e. set the "unknown")
when parsing output of list_projdata_info, explicitly look for the "sum".
This solves a problem when info() statements throw up lots of other things.
The database now contains multiple decays per radioisotope.
This will allow having multiple gamma energies for one radionuclide.
As we include nlohmann/json.hpp in RadionuclideDB.h, it needs
to be a public dependency.
@KrisThielemans
Copy link
Collaborator Author

@danieldeidda I've changed the database format rather drastically. I think this makes more sense and is more extendible. Obviously that needed code changes. Could you have a look please? (I've added a test on Y90, so that seems to work ok).

KrisThielemans and others added 3 commits May 7, 2022 19:42
awk processing depended on info statements. Now only look at lines
that contain min/max
Copy link
Collaborator

@danieldeidda danieldeidda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good I guess I can try this to double check

recon_test_pack/run_test_zoom_image.sh Show resolved Hide resolved
@KrisThielemans
Copy link
Collaborator Author

@danieldeidda let me know if you tested it. I'd like to merge this tomorrow or the day after.

set verbosity level if info() statements to 3, to avoid too much
output
@KrisThielemans
Copy link
Collaborator Author

Merging as tests work, @danieldeidda please create a new issue etc if you discover any problems.

@KrisThielemans KrisThielemans merged commit b6f1c2c into UCL:master May 11, 2022
@KrisThielemans KrisThielemans deleted the RadioNuclideDBnoJSON branch May 11, 2022 20:24
@KrisThielemans KrisThielemans linked an issue May 26, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

radionuclide JSON format RadioNuclideDB looks for JSON files even if we can't read them
2 participants