Skip to content
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
Merged

Add copy constructor and assignment overloading #28

merged 1 commit into from Nov 27, 2017

Conversation

binxiangni
Copy link
Contributor

@binxiangni 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
Copy link
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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

binxiangni commented Nov 14, 2017

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

@LeahPrice
Copy link
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

@adamjohansen adamjohansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Longer-term TODO List
4 participants