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

safe smc::sampler copy constructor #32

Merged
merged 2 commits into from Feb 2, 2018

Conversation

Projects
None yet
4 participants
@mlysy
Contributor

mlysy commented Jan 31, 2018

Dear RcppSMC Developers,

I think this resolves #31 point 1.

mlysy added some commits Jan 30, 2018

Merge pull request #1 from rcppsmc/master
sync to main repo
@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Jan 31, 2018

You're a star. But we are collecting some of the ideas on #31 to have possible tests for students in this year's Google Summer of Code where we posted another proposal. By all means please feel free to chip in if you have additonal ideas, particularly for the earlier "test stage" (ie not too trivial but not to insurmountable) as well as for possible three-month focus ideas.

We'll likely merge the PR but I am a little fried now; we'll see what Adam says in a few...

@adamjohansen

It's probably the case that this gets rid of the memory allocation/deallocation issue, but there are other details which I'm not sure it addresses.

Any thoughts, @LeahPrice or @eddelbuettel ?

// this can only happen if the default adaptMethods was used,
// i.e., no call to SetAdaptMethods
pAdapt = new adaptMethods<Space,Params>;
adaptBelong = 1;

This comment has been minimized.

@adamjohansen

adamjohansen Jan 31, 2018

Collaborator

Don't we also need a
*pAdapt = *sFrom.pAdapt
here?

I'd expect a copy of a sampler to end up in exactly the same configuration as the one that was being copied.

This comment has been minimized.

@LeahPrice

LeahPrice Jan 31, 2018

Collaborator

I would have thought that this one would be okay. If adaptBelong is 1 then SetAdaptMethods hasn't been called at any point so pAdapt is empty. I think the above will produce something that's equivalent but independent.

This comment has been minimized.

@adamjohansen

adamjohansen Jan 31, 2018

Collaborator

That might be true with the current implementation, but there's a certain lack of robustness in assuming you can trace the execution path to the current configuration. Any future changes could break things in a way that would be rather subtle.

In particular, I think it's going to be difficult to address the other issue without introducing a mechanism by which a copied sampler can end up owning pAdapt when the object being copied did not.

As adaptBelong only indicates memory ownership I'd be inclined to include an explicit copy even if it may be redundant at this instant.

// this can only happen if SetAdaptMethods was called,
// i.e., pAdapt points to an external object which should not be deleted with sampler
pAdapt = sFrom.pAdapt;
adaptBelong = 0;

This comment has been minimized.

@adamjohansen

adamjohansen Jan 31, 2018

Collaborator

I'm not sure this is the right behaviour.

If we just copy a pointer like this, then we end up in a situation in which iterating either the original sampler or the new copy changes the configuration of both samplers which is clearly tricky.

It might not be completely trivial to resolve this in an elegant way; there was a reason that this constructor was absent prior to #28 and that was that it's a little delicate to do and not quite clear what the correct behaviour is. Perhaps this pointer to an object model wasn't such a good idea.

This comment has been minimized.

@LeahPrice

LeahPrice Jan 31, 2018

Collaborator

Good point. I find this stuff quite difficult and I'm not sure how to handle it without either having slicing or dependence.

This SO discussion suggests doing a virtual function for the 'clone', but that seems annoying for users.

The suggestions in Bjarne Stroustrup's book seem to be centred around using clone functions too. The options he suggests are along the lines of preventing copying or "make the copy operations private or protected, and provide a clone() function to allow a user to request a non-slicing copy." I.e. using clone as above.

This comment has been minimized.

@adamjohansen

adamjohansen Jan 31, 2018

Collaborator

Well, if we want to allow duplication of derived types from a base class I can't see a way of avoiding a virtual mechanism for doing it so unless there's some cunning way of doing this I'm not sure it can be avoided.

Perhaps it would make sense to agree that this PR at least addresses the memory-safeness (perhaps moduluo the explicit copy I suggested above) and add another item to the list to deal with allowing for proper deep-copies of these objects?

This comment has been minimized.

@LeahPrice

LeahPrice Jan 31, 2018

Collaborator

Okay that seems like a good plan to me as well. I did a quick update to the longer term to-do list but I will leave the GSoC related list for now (in case it's undecided and so you can word it in the way you like for the test)

This comment has been minimized.

@mlysy

mlysy Jan 31, 2018

Contributor

Dear All, thanks for catching this.

Actually I was wondering why SetAlgParam passes by value whereas SetAdaptMethods does not, as the former does bypass this copying problem...

@adamjohansen

This comment has been minimized.

Collaborator

adamjohansen commented Feb 2, 2018

Any objection to us merging this, @eddelbuettel ? I think it's progress but does still leave an open issue of how best to deal with the complications arising from inheritance that I think is probably too complicate for the "easy to address" problems list on reflection.

Also, from a design philosophy point of view, are you happy with me adding a (currently redundant) copy for future robustness as discussed above?

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Feb 2, 2018

Ahh, I may have I misread your comments as more critical than you meant. Yes, incremental is good. Let's merge now and you can tune it as time permits. All good :)

@eddelbuettel eddelbuettel merged commit 2984f35 into rcppsmc:master Feb 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adamjohansen

This comment has been minimized.

Collaborator

adamjohansen commented Feb 2, 2018

I have a tendency to sound more negative than I intend, my fault... there's still an issue with unintended consequences of copying certain types of object, but that's going to be a pain.

While tweaking the code it became obvious I was just trading one sort of future confusion for another, so I'll leave things alone for the time being.

@mlysy mlysy deleted the mlysy:sampler-ctor-patch branch Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment