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

Check load_trace argument #3534

Merged
merged 3 commits into from Jun 28, 2019
Merged

Conversation

rpgoldman
Copy link
Contributor

Still needs a test. But comments welcome.

@rpgoldman
Copy link
Contributor Author

@ColCarroll One piece of advice I could use is what Exception class is right here. I'd like to have a specific one, if only for testing purposes...

@rpgoldman
Copy link
Contributor Author

@ColCarroll One piece of advice I could use is what Exception class is right here. I'd like to have a specific one, if only for testing purposes...

I suppose ValueError would be reasonable, since we have a string that correctly names a location in the filesystem, but it's not a trace.
Adding LoadTraceError would be a reasonable extension to PyMC3, I suppose.

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

Looks good -- one nitpick, plus I agree that ValueError or a custom error (that inherits from ValueError) is a better idea.

for subdir in glob.glob(os.path.join(directory, '*')):
if os.path.isdir(subdir):
straces.append(SerializeNDArray(subdir).load(model))
if straces == []:
Copy link
Member

Choose a reason for hiding this comment

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

should be if len(straces) == 0 (or if not straces), since testing list equality is dangerous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Everything should be in place now. ISTR getting a pylint complaint about checking length as a way to check for an empty list, so I went for if not straces.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I hate that warning!

Add new exception class.
While doing this, added type annotations to ndarray.py
@rpgoldman rpgoldman changed the title WIP: Check load_trace argument Check load_trace argument Jun 28, 2019
@@ -7,3 +7,7 @@ class SamplingError(RuntimeError):

class IncorrectArgumentsError(ValueError):
pass

class TraceDirectoryError(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

nice. i did not know this file existed!

@ColCarroll
Copy link
Member

I'll merge once tests pass -- thanks!

@@ -204,7 +215,7 @@ def setup(self, draws, chain, sampler_vars=None):
if self._stats is None:
self._stats = []
for sampler in sampler_vars:
data = dict()
data: Dict[str, np.ndarray] = dict()
Copy link
Member

Choose a reason for hiding this comment

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

sorry, this line is too current for python 3.5...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, crumbs. I keep forgetting. I'll replace that with the corresponding type comment. My bad.

This commit should be squashed before merging...
@rpgoldman rpgoldman merged commit ff7d3cd into pymc-devs:master Jun 28, 2019
@rpgoldman rpgoldman deleted the check-load-trace branch June 28, 2019 15:53
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.

None yet

2 participants