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

Multiple Energy Window Code #722

Open
wants to merge 290 commits into
base: master
Choose a base branch
from

Conversation

ludovicabrusaferri
Copy link

This is a Pull Request for merging the multiple energy window code into master

@KrisThielemans
Copy link
Collaborator

Travis fails because of undefined virtual energy functions for CListModeDataGEHDF5. Probably easiest not to derive from CListEnergy?

I didn't check why Appveyor or codacy failed...

@ludovicabrusaferri
Copy link
Author

Ok I have removed the dependency. I guess it will be the same for CListRecordECAT too? Let me know.

@KrisThielemans
Copy link
Collaborator

Possibly we cannot just delete it as LmToProjData uses record.energy().get_energyA_in_keV().

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?

@KrisThielemans
Copy link
Collaborator

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...

@danieldeidda
Copy link
Collaborator

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?

@KrisThielemans
Copy link
Collaborator

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 Bin I think. That way, it seems we can let LmToProjData be agnostic about energy windows. I haven't thought about this properly though...

@ludovicabrusaferri
Copy link
Author

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

@KrisThielemans
Copy link
Collaborator

Travis: seems some things are failing with the recon_test_pack: ROOT support test scripts (e.g. run_ROOT_gate.sh) but also run_scatter_tests.sh (although that only with error relative to sup-norm of first data-set = 0.435529 % at the very end)

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 ExamInfo) but I found

C:\projects\stir\src\scatter_buildblock\scatter_detection_modelling.cxx(244): error C2065: 'M_PI': undeclared identifier [C:\projects\stir\build\src\scatter_buildblock\scatter_buildblock.vcxproj]
C:\projects\stir\src\scatter_buildblock\scatter_detection_modelling.cxx(244): error C2789: 'den1': an object of const-qualified type must be initialized [C:\projects\stir\build\src\scatter_buildblock\scatter_buildblock.vcxproj]

For the first error, use _PI (M_PI isn't portable, _PI is defined in stir/common.h which is always included). The 2nd is probably just a consequence of the first.

Comment on lines +139 to +140
energyA = energy1;
energyB = energy2;
Copy link
Collaborator

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.

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.

None yet

4 participants