[CCPPETMR/SIRF] New data container sptr (#254)
Richard Brown
In SIRFReg, we store shared pointers to ImageData. The output of the resampling/registration should be the same as the reference image, which means we need a virtual method that will create a shared pointer of any class that derives from ImageData.
Depends on #253.
You can view, comment on, or merge this pull request online at:
https://github.com/CCPPETMR/SIRF/pull/254
Commit Summary
- Mark DataContainer methods const where possible
- new_data_container_sptr
File Changes
- M src/common/include/sirf/common/DataContainer.h (13)
- M src/common/include/sirf/common/ImageData.h (4)
- M src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp (16)
- M src/xGadgetron/cGadgetron/gadgetron_data_containers.h (104)
- M src/xSTIR/cSTIR/stir_data_containers.cpp (10)
- M src/xSTIR/cSTIR/stir_data_containers.h (47)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
evgueni-ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
strongly against - i spent some time to make DataContainer independent of shared_ptr used (std or boost)
can you not instead create shared_ptr in your code from the pointer returned by new_data_container()?
evgueni-ovtchinnikov
@evgueni-ovtchinnikov requested changes on this pull request.
strongly object to data container sptr in DataContainer - see below
mr acquisition container algebra methods cannot be const because they are based on get_acquisition which in turn calls ISMRMRD::Dataset.readAcquisition in the case of acquisitions from file
Richard Brown
@rijobro commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
I thought about that, but I don't think I can do that (unless I try to dynamic_cast to all derived classes.
What is the rationale for getting rid of shared pointers? I know we wanted to get rid of boost, but std is OK, isn't it?
evgueni-ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
boost::shared_ptr is still an option in STIR
could you not use this function instead?
std::shared_ptr<DataContainer> new_data_container_sptr(const DataContainer& dc)
{
return std::shared_ptr<DataContainer>(dc.new_data_container());
}
Kris Thielemans
we agreed to no longer support boost::shared_ptr for STIR. We need C++-11 anyway, so we just say that STIR needs to be compiled with C++-11.
But the trick might work ok?
Richard Brown
@rijobro commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
And just put it in DataContainer.h instead of all the derived classes? Good point.
evgueni-ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
to cover both shared_ptr, i would do:
'''
void get_new_data_container_sptr(const DataContainer& dc, std::shared_ptr& sptr)
{
sptr = std::shared_ptr(dc.new_data_container());
}
'''
Richard Brown
@rijobro commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
Fine by me. But I suppose we should only implement the first of those two for the time being? Since we're trying to move away from boost?
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
no please. no boost.
a get method has to return something. Otherwise it's have to be called fill or so (in which case the output has to come first).
Richard Brown
@rijobro commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
fill_shared_ptr_with_new_data_container(). It just rolls of the tongue!
evgueni-ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
get: i for one always prefer to use a verb for void function (do_something_with(x);) and noun for a one that returns something (x = a_thing();) (cf. set_up vs setup)
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
I'm sorry. I see no need for the fill. I'm happy with the first proposal
std::shared_ptr<DataContainer> get_new_data_container_sptr() const
{
return std::shared_ptr<DataContainer>(this->new_data_container());
}
probably use make_shared
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
good convention with the verb and noun, but sadly get is so standard that we have to stick to it (I'm hoping we do in other places)
Richard Brown
@rijobro commented on this pull request.
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,12 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const = 0;
Ok, reverted to return std::shared_ptr<DataContainer>(new_data_container());. Can't use make_shared as that calls the object's contructor.
Richard Brown
@rijobro pushed 5 commits.
- f1b75bf Merge branch 'master' into datacontainer_const_methods
- cf9c013 Clear up whitespaces
- 46b648f Clear up whitespaces
- 308d554 Merge branch 'datacontainer_const_methods' into new_data_container_sptr
- b5403be Test new_data_container_sptr()
—
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
2 changes and merge master
In src/common/include/sirf/common/DataContainer.h:
> @@ -41,11 +41,15 @@ namespace sirf {
class DataContainer {
public:
virtual ~DataContainer() {}
- virtual DataContainer* new_data_container() = 0;
- virtual ObjectHandle<DataContainer>* new_data_container_handle() = 0;
- virtual unsigned int items() = 0;
- virtual float norm() = 0;
- virtual void dot(const DataContainer& dc, void* ptr) = 0;
+ virtual DataContainer* new_data_container() const = 0;
+ virtual std::shared_ptr<DataContainer> new_data_container_sptr() const
⬇️ Suggested change
- virtual std::shared_ptr<DataContainer> new_data_container_sptr() const + std::shared_ptr<DataContainer> new_data_container_sptr() const
In src/common/include/sirf/common/ImageData.h:
> @@ -39,7 +39,7 @@ namespace sirf {
virtual Iterator_const& begin() const = 0;
virtual Iterator& end() = 0;
virtual Iterator_const& end() const = 0;
- void copy(Iterator_const& src, Iterator& dst, Iterator& end)
+ void copy(Iterator_const& src, Iterator& dst, Iterator& end) const
⬇️ Suggested change
- void copy(Iterator_const& src, Iterator& dst, Iterator& end) const + static void copy(Iterator_const& src, Iterator& dst, Iterator& end)
Kris Thielemans
@KrisThielemans requested changes on this pull request.
Needs a conflict resolution, and a tiny suggestion.
However, given #278, I think we should call new_data_container_sptr simply clone, while leaving new_data_container as-is for now until we agree on #278. As far as I understand this won't interfere with MRAcquisitionData::clone (but I would make that const as well).
In src/xSTIR/cSTIR/tests/test4.cpp:
> @@ -37,6 +37,16 @@ int test4() converter.set_time_interval(0, 10); converter.set_up(); converter.estimate_randoms(); + + // new data container sptr + string f_image = path + "mu_map.hv"; + STIRImageData image(f_image); + std::shared_ptr<ImageData> copy_as_sptr = + std::dynamic_pointer_cast<ImageData>(image.new_data_container_sptr()); + std::shared_ptr<STIRImageData> copy = std::dynamic_pointer_cast<STIRImageData>(copy_as_sptr);
could add a check on content
Richard Brown
Closing as superceded by #277.
Richard Brown
Closed #254.