Skip to content

Conversation

@rpgoldman
Copy link
Contributor

Still needs a test. But comments welcome.

@rpgoldman rpgoldman requested a review from ColCarroll June 27, 2019 20:28
@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.

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
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!

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.

2 participants