remove presmoothingforward & postsmoothingback projectors #607
Conversation
There was a problem hiding this comment.
Do we really need to implement most of these copy constructors? Isn't the default one ok in most cases?
It's seems surprising to let create_shared_clone() return a unique_ptr.... What was our terminology for the other hierarchy (I can't find it anymore...).
Otherwise looks good. I guess we need to still add a few others.
| @@ -55,6 +55,12 @@ BackProjectorByBin::BackProjectorByBin() | |||
| set_defaults(); | |||
| } | |||
|
|
|||
| BackProjectorByBin::BackProjectorByBin(const BackProjectorByBin& to_copy) | |||
| : _already_set_up(false), _density_sptr(nullptr), _post_data_processor_sptr(nullptr) | |||
There was a problem hiding this comment.
why set these to null etc? I can agree you'd want to force people to call set_up again (although I'm not sure it's strictly necessary), but I find it dangerous to remove any _post_data_processor_sptr. If there was one, it won't be a clone anymore!
There was a problem hiding this comment.
I thought it best to set them as null so as not to create a shallow copy (since they don't have their own create_shared_clone methods). What if you think you're modifying the data processors parameters for the clone, when in actual fact, you're modifying the parameters of both the clone and the original?
There was a problem hiding this comment.
That sounds dangerous indeed. However, the question is if it can happen. We've put in a lot of effort to avoid it in the other stuff.
However, as far as I remember, _density_sptr is only set by BackProjector::set_up() which would create a new sptr. (not modify an existing one). Indeed, I don't think there's a way to modify to modify the shared object. Similarly, as long as we don't have a shared_ptr<DataProcessor> get_post_data_processor_sptr() or DataProcessor& get_post_data_processor(), I cannot see how it would be unsafe to reuse it.
It's possibly a bit confusing that set_post_data_processor would override anything that was already there. We replace that then with add (and use ChainedDataProcessor) and add a remove_post_data_processor like I believe you do in SIRF transformations. However, I don't think that's really necessary for now.
So, if we don't do that work, the question is what is more surprising: a clone which doesn't do the same (the current situation), or a clone where a subsequent set_post_data_processor would override whatever was there. I certainly think the former is less desirable.
There was a problem hiding this comment.
the
unique_ptrwas functionality that I copied from SIRF'sclonemethods forDataContainer. Would you rather it wereshared_ptr? I thoughtunique_ptrwould let the user know that the copy wasn't stored elsewhere.
That's true, but it's weird to have a create_shared return a unique. It should then be called create_unique_clone. I'm fine with calling it the latter. (It's certainly better than implying that its properties are shared)
There was a problem hiding this comment.
2nd thoughts. Are they in fact unique?
For instance, the projmatrix presumably still is shared (I would certainly suggest that it is, as otherwise you're going to run out of memory very quickly). Can anyone call its various set functions? Obviously, the original owner could. This is an example where sharing is actually necessary, albeit it dangerous. oh well
This is
There was a problem hiding this comment.
It's confusing, I tried to make them as independent as possible (e.g., no sharing of data processors), but they do share the projmatrix (for memory).
My feeling is that we can do what we want in terms of which objects are copied shallowly and deeply, provided we document it well. I already put in the doxygen that the projmatrix would only be shallow copied. I can also do a shallow copy of the data processor if you'd like, and document that, too.
We could name the method create_clone, which is less specific on unique versus shared. This seems appropriate, since we're doing a mix of the two.
What do you think?
| @@ -56,6 +56,12 @@ ForwardProjectorByBin::ForwardProjectorByBin() | |||
| set_defaults(); | |||
| } | |||
|
|
|||
| ForwardProjectorByBin::ForwardProjectorByBin(const ForwardProjectorByBin &to_copy) | |||
| : _already_set_up(false), _density_sptr(nullptr), _pre_data_processor_sptr(nullptr) | |||
There was a problem hiding this comment.
see comment for BackProjectorByBin
|
the |
If we use the default, then we'll create a shallow copy of any shared pointers, which seems dangerous to me. Ideally, both
It looks like, currently, this functionality would only be used in #580 and then #240 once I get that up to date again. Could we do it on a case-by-case basis? |
discussed above. I don't think that's necessary here. Obviously, the aim of having |
Co-authored-by: Kris Thielemans <[email protected]>
Suggestion fix for #606. So far, I've only implemented copies for
...MatrixByBinprojectors, but would have to be implemented for all projectors. Could easily be extended to projector pairs.