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

Add copy constructor and assignment overloading #28

Merged
merged 1 commit into from Nov 27, 2017

Conversation

Projects
None yet
4 participants
@binxiangni
Contributor

binxiangni commented Nov 14, 2017

Learning some computational statistics methods this semester and trying to apply my knowledge to fix #25. One question: how do you do unit tests in this package?

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Nov 14, 2017

Sorry, I am lost: where does #25 say anything about a copy constructor?

In general, I am a fan of first opening issue tickets, discussing what you want to do and getting consensus for it to get done. Throwing "random" pull request over the fence and asking us to review is a little risky as everybody is busy.

@binxiangni

This comment has been minimized.

Contributor

binxiangni commented Nov 14, 2017

Sorry, I'll open a new issue next time to contribute. The fourth item of the checklist in #25 is Add sampler copy constructor.

@eddelbuettel

This comment has been minimized.

Collaborator

eddelbuettel commented Nov 14, 2017

Got it. Overlooked this.

This project is now a bit of a continuation of Leah's GSoC. If you have spare cycles, I may have something else (or it may be too small...).

@binxiangni

This comment has been minimized.

Contributor

binxiangni commented Nov 14, 2017

I have a long Thanksgiving break and trying to find something to work on :)

@LeahPrice

This comment has been minimized.

Collaborator

LeahPrice commented Nov 27, 2017

This looks good to me, thanks Binxiang! Sorry I lost track of this on my to do list - I hope you've still got some time left on your break! It would be good to have you involved if you'd like to be. I'd also be happy to chat about computational statistics if you'd like. My PhD is in comp stats.

@LeahPrice LeahPrice merged commit 01b01e1 into rcppsmc:master Nov 27, 2017

pPopulation = sFrom.pPopulation;
///The set of moves available.
Moves = sFrom.Moves;
/// The additional algorithm parameters.

This comment has been minimized.

@adamjohansen

adamjohansen Nov 27, 2017

Collaborator

Sorry for being rather late with this, I've been swamped with other things.

Thanks for this, Binxinag, it's always good to sort out these small issues.

However, there is a bit of a complication, at this stage the code copies a pointer and also an indicator of whether the object pointed to belongs to a sampler. If you copy a sampler which owns pAdapt and delete both of them then they will both delete the same object.

I suspect that the cleanest option would be to do a deep copy of the object pointed to and then to set adaptBelong to TRUE regardless of what the original configuration was, but I haven't thought very much about this.

Any other thoughts on this?

This comment has been minimized.

@binxiangni

binxiangni Nov 27, 2017

Contributor

Got it. Perhaps we could do it like

 *pAdapt = *sFrom.pAdapt;
 adaptBelong = sFrom.adaptBelong;

Then even if adaptBelong == true, we do not delete the same object.

This comment has been minimized.

@binxiangni

binxiangni Nov 27, 2017

Contributor

Then one new problem may come out. When pAdapt == null at first, *pAdapt may bring segment fault. :(

This comment has been minimized.

@adamjohansen

adamjohansen Nov 27, 2017

Collaborator

Quite so. This is where pAdapt would be initialised so to go down that route you'd need to allocate memory before assigning to it -- but that wouldn't need much more than an additional
pAdapt = new...
assuming that the deep copy model is the approach we want to go with here.

Also, if we do do this then it should probably be
adaptBelong = true
rather than
adaptBelong = sFrom.adaptBelong
because even if the source object doesn't own the object pointed to by its pAdapt the copied object does.

This comment has been minimized.

@LeahPrice

LeahPrice Nov 27, 2017

Collaborator

Thanks Adam for picking up on this. I'd forgotten about this issue of dealing with the pointer in the copy. I think I put this off because I didn't have a great solution at the time but I like the sound of your solution.

This comment has been minimized.

@adamjohansen

adamjohansen Nov 28, 2017

Collaborator

There's been a history of omitting the copy constructor because it was fiddly for various reasons dating right back to the first implementation. I think we're closing in on something that will at least work -- it will be great to have something in place, but we should remember to be clear in any documentation exactly what the behaviour on copying is as it might not be quite what is expected.

In particular, when the object pointed to by pAdapt is a derived class we'll call the base class constructor / assignment operator if we go down this route and the copied sampler won't behave in quite the same way as the original. The user can address this by explicitly setting the value of this pointer, as long as they're aware that they need to.

(The only obvious alternative that I can see would be to always copy the pointer and then set adaptBelong = false but that runs into problems if the original sampler owned the object and is destroyed before the copy... but if anyone has other thoughts they'd be very welcome.)

@adamjohansen

I think there is an issue with memory management here that we should remember to address.

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