-
Notifications
You must be signed in to change notification settings - Fork 95
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
Multiple Energy Window Code #722
base: master
Are you sure you want to change the base?
Conversation
Travis fails because of undefined virtual energy functions for I didn't check why Appveyor or codacy failed... |
Ok I have removed the dependency. I guess it will be the same for CListRecordECAT too? Let me know. |
Possibly we cannot just delete it as The only way I see to resolve that is by doing thing like // before the loop, do a `dynamic_cast` to test if we have energy
auto clist_energy_ptr = dynamic_cast<CListEnergy const *> (record);
for (...)
{
if (clist_energy_ptr)
{
// do checks on energy
}
} a bit ugly. Maybe you see another way? |
Another problem is a much bigger one: what do we do with SPECT where there's only 1 energy window. Maybe @danieldeidda has some great ideas... |
sorry I need to read through this PR, but why 1 energy window would be a problem? |
a lot of the code talks about pairs. This is unavoidable (and correct) for scatter, but will be a pain for SPECT, as there it doesn't make sense. Somehow we'd have to "hide" it I guess. Or replace a "pair" with a single index. We could shift the single "pair" index into |
I can see indeed this could be an issue for spect. I need to think about it carefully too , I'll come back to this asap |
Travis: seems some things are failing with the Appveyor: a lot of warnings make it hard to see where the errors are (some are due to main STIR, so I'll fix the main culprit in
For the first error, use |
energyA = energy1; | ||
energyB = energy2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #736, I cant figure out the benifit having energyA
adn energyB
.
This is a Pull Request for merging the multiple energy window code into master