MAINT: make sure copy links weights and optimizer

[stsievert]

Closes #317.

This needs review, hence the WIP label. I don't use state_dict as mentioned in #317 (comment), and I coded to the test.

[benjamin-work]

Thank you for taking on this issue.

The fix looks sound. The use of state_dict could help with portability but that is another issue.

There are some adjustments required for the unit test:

• As it is right now, it succeeds even without the fix. The reason is that you call fit the second time instead of partial_fit (always first check whether the test fails before writing the bug fix).
• The test name should be more explanatory.
• The unit test definitely needs more explanations in the form of comments, e.g. it is not restricted to deep copying but would also occur with pickle. You can also link to the issue in a comment.
• Finally, it is not quite clear whether all of the asserts are needed. Something like this directly after the deepcopy already reveals the bug:

        # check that the optimizer references the same parameters as used by 
        # the module
        for p0, p1 in zip(
            n2.module_.parameters(), n2.optimizer_.param_groups[0]['params']
        ):
            assert p0 is p1

But I also see the value in comparing the parameters after the fit call, so leave that in as well. The comparison of train_loss, on the other hand, seems pointless -- if the module parameters are updated, can the loss ever the same (and if it were, the issue would certainly lie somewhere else)?

[stsievert]

Comments addressed. I've added a lot more comments and this seems like a better test.

The comparison of train_loss, on the other hand, seems pointless -- if the module parameters are updated, can the loss ever the same (and if it were, the issue would certainly lie somewhere else)?

This is a user-facing test, and the reason I filed this issue and PR. I agree that it's redundant, but if that assert fails users will complain, and it's a good sanity check.

[benjamin-work]

Thanks for the changes, the test is now much better to understand. There are only 2 minor comments left.

[benjamin-work]

Typo "cal"; also, why are 2 calls needed? When I comment one out, the test still passes.

[stsievert]

Thanks for this. I can also pass epochs=2 to partial_fit, and I explained more why in the comments.

[benjamin-work]

This line is too long, could you please break it?

[benjamin-work]

Thank you very much for this contribution.