Pull request for merging LMU modifications#41
Conversation
|
I have converted I'm not sure what needs to be done with I don't fully remember what was said about those scripts, sorry. |
|
@SimonRit can I rebase on top of |
Sure |
acoussat
force-pushed
the
LMU_HeCT
branch
3 times, most recently
from
0f7421e to
1a9815d
Compare
|
I think the PR is almost ready, the only thing I could still see missing is some documentation. Should I try to add some, should we ask George to write some, or do we consider the new features as niche and just skip the documentation? |
Thanks. We should document all the functionalities but that can come later. Reviewing the PR (which I can't approve as it's mine!), I wonder if it makes sense to have the two applications |
|
I completely agree and I even thought about merging the two codes into one when implementing it. The problem is that |
|
The functionalities in pctfilterprotons should be merged in pctpaircuts before merging. |
acoussat
force-pushed
the
LMU_HeCT
branch
2 times, most recently
from
b6e47c7 to
fb627df
Compare
| #include <itkTimeProbe.h> | ||
|
|
||
| /** \class FDKDDConeBeamVarianceReconstructionFilter | ||
| * TODO |
There was a problem hiding this comment.
Missing comment here, should I try and make up one?
There was a problem hiding this comment.
Yes, e.g., "Implements variance reconstruction with distance-driven filtered backprojection reconstruction (doi:10.1118/1.4789589) from variance projections." Thanks!
There was a problem hiding this comment.
Also missing from FDKDDConeBeamReconstructionFilter but I guess this is unrelated to this PR and can go in a separate PR in which we clean and improve the documentation.
There was a problem hiding this comment.
Opened #85 which cleans a bit the C++ docstrings.
acoussat
changed the title
| // this->m_RamLakCutFrequency = 0.; | ||
| // this->m_SheppLoganCutFrequency = 0.; | ||
| } | ||
| {} |
There was a problem hiding this comment.
Can you use the =default constructor instead ? It can be in the header I believe (see InsightSoftwareConsortium/ITK#6000 otherwise)
There was a problem hiding this comment.
I'm not entirely familiar with the exact difference between {} and = default… Should I also update other instances such as the constructors of EnergyStragglingFunctor, FDKDDWeightProjectionFilter, MostLikelyPathFunction and so on?
There was a problem hiding this comment.
Can be done in a separate PR yes. You can check SO for some explanations on the difference but to me it's mainly a matter of style, much less code.
Amended by Simon Rit for merging and avoiding inclusion of _ggo.h files
|
If you agree, please merge and address the comments in separate PRs for the whole repository. |
2 participants
No description provided.