Conversation
There is a weird point, as in order to get to the version I need to skip to byte 32 and read it as big endian. The header kind and header size are read correctly, as little endians in the right place. In the print_header I cannot any relevant process.
Progress on matters of initialisations using SimSET headers. Next step, initialise the history header and get events.
Trying to guess the scanner.
Test pending. Todo: fix the signature. Now it is always on.
I am trying to make the old simSET example to work. Had to modify slightly the binning module to match with STIR scanner template. But the error of the different axial length persists. I am trying to guess the geometry with even fewer data.
Fixed the mCT block config
KrisThielemans
left a comment
There was a problem hiding this comment.
some initial comments. What's in the SimSET.tar.gz? Can't check in source code.
|
|
||
| #endif | ||
|
|
||
| // This is an alternative version of the is_SimSET_signature. |
There was a problem hiding this comment.
I will have removed it by the end of this PR
src/include/stir/listmode/CListEventCylindricalScannerWithDiscreteDetectors.inl
Outdated
Show resolved
Hide resolved
| endif() | ||
|
|
||
| if (HAVE_SIMSET) | ||
| list(APPEND ${dir_LIB_SOURCES} |
There was a problem hiding this comment.
will probably need changing
| // uncompressed bins in a compressed bin | ||
|
|
||
| const float bin_value = 1.f / bin_efficiency; | ||
| const float bin_value = uncompressed_bin.get_bin_value() / bin_efficiency; |
There was a problem hiding this comment.
this seems dangerous for backwards compatibility and will need careful checking
There was a problem hiding this comment.
The problem is that SimSET events carry can carry weights and we need those.
There was a problem hiding this comment.
Either in the cylidrical or the blocks geometry, I am not 100% sure in which the weights were applicable.
| # NOTE: Take note if you are setting min_td and max_td to any value less than the radius of your object cylinder. | ||
| # num_td_bins needs to be odd for STIR, and the range needs to be symmetric | ||
| INT num_td_bins = 287 | ||
| INT num_td_bins = 288 |
There was a problem hiding this comment.
see comment above. For even-sized, I seem to recall there was a difference between SimSET and STIR conventions
There was a problem hiding this comment.
Probably. But that is why the TOF modification from PETSIRD is relevant, here. IDK what to do. Bring that over and resolve the conflict later? Which one will be merged first?
There was a problem hiding this comment.
This has nothing to do with TOF. It is tangential positions.
In any case, and "even TOF" mods should not be in either of these PRs, so they should be independent. To be discussed tomorrow what we do with the even TOF bins (with @danieldeidda)
There was a problem hiding this comment.
oops.. I missread. Let's keep this open I don't remeber right now.
|
OK, solid progress on this one. We are able to compile the latest master with SimSET support. I have not tested yet. |
KrisThielemans
left a comment
There was a problem hiding this comment.
some house-keeping comments, nothing really regarding functionality.
I suggest moving most (all?) of .inl to .cxx to .cxx. There seems to be no reason to think any compiler will be able to inline all of that code (and the remaining ones are probably hardly ever used anyway)
| target_compile_definitions(SIMSET::simset INTERFACE DARWIN MAC_10 GEN_UNIX arm64) | ||
| elseif(UNIX) | ||
| target_compile_definitions(SIMSET::simset INTERFACE GEN_UNIX) | ||
| elseif(WIN32) | ||
| target_compile_definitions(SIMSET::simset INTERFACE WIN32) | ||
| endif() |
There was a problem hiding this comment.
fix indentation, but I'm hoping we don't need these (They are needed for Simset compilation, but do we need it for the Simset headers?) Add a comment if so.
There was a problem hiding this comment.
Photon.h uses types like LbFourByte, LbUsFourByte, etc. Those are defined in SimSET’s LbTypes.h inside an OS-specific #if ... #elif ... block. If none of the expected platform macros are defined, you get “unknown type name LbFourByte” (or even #error you have not declared an OS-dependent clause). This can be changed in the SimSET code. No super hard, but we need to wait until the public version.
| START_NAMESPACE_STIR | ||
|
|
||
| //! N.E: Large parts adapted from phgbin and functions called by it; | ||
| CListModeDataSimSET::CListModeDataSimSET(const std::string& _hsimset_filename) |
There was a problem hiding this comment.
either we move all/most of this into open_lm_file (which could be useful if at some point we want to be able to call it from somewhere else), or we delete open_lm_file
Changes in this pull request
This PR add support for SimSET history files.
In order to make this run you need to link to SimSET.
Apply this patch to SimSET and recompile.
This will crate a lib file that you can point to at CMake.
Testing performed
Related issues
Checklist before requesting a review
documentation/release_XXX.mdhas been updated with any functionality change (if applicable)Contribution Notes
Please tick the following: