Skip to content

Comments

Add save/load ndarray to release notes, small bug fix#2981

Merged
junpenglao merged 2 commits intopymc-devs:masterfrom
ColCarroll:fix_save_ndarray
May 21, 2018
Merged

Add save/load ndarray to release notes, small bug fix#2981
junpenglao merged 2 commits intopymc-devs:masterfrom
ColCarroll:fix_save_ndarray

Conversation

@ColCarroll
Copy link
Member

Added a test and a fix for a bug in save/load_trace, and added a note to RELEASE_NOTES.md

Copy link
Member

Choose a reason for hiding this comment

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

Should we ask or alert before removing directories?

Copy link
Member Author

Choose a reason for hiding this comment

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

This scares me too, but my intention here is to treat self.directory as a single filesystem object that pymc3 owns. In this context, it is not unusual to overwrite without asking/alerting. I think this is fine, but I could see something ugly happening if you call pm.save_trace(trace, './'), intending to save a file to the current directory.

My best "safe" alternative that is not overly onerous is to write .pymc.manifest to the top level of the directory, with a list of all the files that pymc is allowed to delete in the future. Then this deletion will be just running over all the files in the existing manifest and deleting those. In that case, I would throw an exception then if any path already exists after deleting files in the manifest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even more explicitly, by default, there would be a file ./.pymc3.trace/.pymc.manifest, whose contents are something like

0/samples.npz
0/metadata.json
1/samples.npz
1/metadata.json
...

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could do add overwrite=False flag that if False and the dir exists, we raise an exception that you have to set overwrite=True.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's reasonable, though I always appreciate programs where save "just works". what if i followed jupyter's lead and did .pymc3_1.trace, .pymc3_2.trace, etc. by default?

Copy link
Member

Choose a reason for hiding this comment

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

I would imagine that this saving is done multiple times for a single model, so overwriting is probably a useful feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, i agree with that! I am suggesting that the signature be

def save_trace(trace, filename=None, overwrite=False):

if filename is None:
    # find a unique filename to write to 
else:
    if filename exists:
        if overwrite is False:
            raise Exception
        else:
            # clear out directory

Copy link
Member

Choose a reason for hiding this comment

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

ok, that sounds great!

@ColCarroll
Copy link
Member Author

I think this is ready to go now.

@junpenglao junpenglao merged commit af05150 into pymc-devs:master May 21, 2018
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.

3 participants