WIP: refactoring of the code base #226
Conversation
include/dcmqi/DICOMFrame.h
Outdated
|
|
||
| DICOMFrame(DcmDataset *dataset, int number) : | ||
| frameNumber(number), | ||
| frameDataset(dataset) { |
| typedef short ShortPixelType; | ||
| typedef itk::Image<ShortPixelType, 3> ShortImageType; | ||
| typedef itk::ImageFileReader<ShortImageType> ShortReaderType; | ||
|
|
||
| namespace dcmqi { | ||
|
|
||
| class ConverterBase { | ||
| class MultiframeConverter { |
There was a problem hiding this comment.
MultiframeConverter should be MultiFrameConverter
ifdefs should be renamed according to class name
#ifndef DCMQI_CONVERTERBASE_H
#define DCMQI_CONVERTERBASE_H
There was a problem hiding this comment.
The filenames use "Multiframe" but some of the code uses "MultiFrame" so it should be uniform in the codebase.
The DICOM standard uses "Multi-frame" which can't be used as an identifier, so that doesn't help.
It looks like DCMTK has a mix of MultiFrame and Multiframe.
Personally I prefer Multiframe and would suggest using that consistently and that looks like Andrey's preference too.
There was a problem hiding this comment.
@michaelonken is there a preference in DCMTK for Multiframe vs. MultiFrame?
I see several examples of both currently...
(cd DCMTK; git grep -i multiframe) | less
There was a problem hiding this comment.
I did pay attention to this, and my logic is to capitalize individual words. Since they are not separate words in the standard, I decided not to capitalize Frame.
| * | ||
| * | ||
| */ | ||
| class MultiframeObject { |
There was a problem hiding this comment.
MultiframeObject --> MultiFrameObject
|
Your English instincts are quite good Andrey. In general English usage if
a hyphenated word gets a lot of usage the hyphen eventually gets dropped so
let's do go with Multiframe everywhere.
…On Thu, Apr 6, 2017 at 12:25 PM, Andrey Fedorov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/dcmqi/MultiframeConverter.h
<#226 (comment)>:
> typedef short ShortPixelType;
typedef itk::Image<ShortPixelType, 3> ShortImageType;
typedef itk::ImageFileReader<ShortImageType> ShortReaderType;
namespace dcmqi {
- class ConverterBase {
+ class MultiframeConverter {
I did pay attention to this, and my logic is to capitalize individual
words. Since they are not separate words in the standard, I decided not to
capitalize Frame.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#226 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHsfdAF4KgCdPjnRDhCfr4qwXGuV09Gks5rtRHjgaJpZM4MZ4Aa>
.
|
fedorov
force-pushed
the
frame-handling-refactoring
branch
from
2c1aa3b to
9fdbc16
Compare
fedorov
force-pushed
the
frame-handling-refactoring
branch
from
4466a5d to
e1827fe
Compare
fedorov
force-pushed
the
frame-handling-refactoring
branch
from
3baebf7 to
70b0055
Compare
libsrc/ParametricMapObject.cpp
Outdated
| initializeContentIdentification(); | ||
|
|
||
| // TODO: consider creating parametric map object after all FGs are initialized instead | ||
| createParametricMap(); |
There was a problem hiding this comment.
Even though createParametricMap is called within initializeFromITK, I think the method name should indicate the type (DICOM or ITK)
We could add two virtual methods {create/get}{DICOM/ITK}Representation to MultiframeObject that needs to be implemented by inheriting classes.
P.S. with initializeFromITK I associate reading an ITK image into the internal data structure. I don't associate the conversion into DICOM with it.
fedorov
force-pushed
the
frame-handling-refactoring
branch
3 times, most recently
from
3ed27af to
f8486ba
Compare
This is an intermediate commit that aims to improve organization of the conversion functionality, motivated by the various standing issues (e.g., QIICR#217, QIICR#192 and QIICR#150) that are difficult to address with the current level of disarray in the code. Some of the ideas are the following: - introduce basic hierarchy of classes to allow reuse of the common functionality between PM and SEG converters (MultiFrameConverter parent for ParametricMapConverter and SegmentationImageConverter) - do not use static functions, instead make better use of converter classes - factor out containers to keep information about ImageVolume and DICOMFrame to simplify conversion and book keeping - improve consistency of naming and code style My plan to proceed with the refactoring is to implement "shadow" functions in the updated classes while keeping the original functions initially, and then phase out the original functions. The current commit is just an intermediate milestone to open this for feedback and keep development history at least somewhat incremental.
* added Helper class static methods
itkimage2paramap tests are passing!
crash in addForAllFrames()!...
Also: * fix the typo in JSON lookup * replace the temporary codes with the standard ones: CP-1665 became standard in 2017a
CP-1665 became standard in 2017a
dciodvfy is happy!
rows/columns were flipped on initialization
* added replacement method to SegmentationImageConverter
* TODO: refactor even more and generalize
- PixelContrast is no longer needed for PM - use standard code for fitting model type
Round-trip tests are now passing
If DerivationImageSequence is not empty for at least one frame, it must be present for each of the frames.
fedorov
force-pushed
the
frame-handling-refactoring
branch
from
49ed047 to
ec36add
Compare
ENH: reusing existing components, reorganizing, volume extent computation fix
WIP == FYI == "do not even consider merging"
This is an intermediate commit that aims to improve organization of the conversion
functionality, motivated by the various standing issues (e.g., #217, #192 and #150)
that are difficult to address with the current level of disarray in the code.
Some of the ideas are the following:
functionality between PM and SEG converters (MultiFrameConverter parent for
ParametricMapConverter and SegmentationImageConverter)
to simplify conversion and book keeping
My plan to proceed with the refactoring is to implement "shadow" functions in the
updated classes while keeping the original functions initially, and then phase out
the original functions.
The current commit is just an intermediate milestone to open this for feedback
and keep development history at least somewhat incremental.