Update AutoCorrPlot test #282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
canyon289
force-pushed
the
autocorrplot_test
branch
from
46ad333
to
f11b986
Compare
|
Having plotting and conversion as separated tests is cleaner, but I am afraid this could lead to miss errors. I prefer to be overly-caution and keep the tests as they are right now as plots are intended to interact with PyMC3 and Stan objects and not |
|
Sounds good to me!
Thanks for the input
…On Tuesday, September 25, 2018, Osvaldo Martin ***@***.***> wrote:
Having plotting and conversion as separated tests is cleaner, but I am
afraid this could lead to miss errors. I prefer to be overly-caution and
keep the tests as they are right now as plots are intended to interact with
PyMC3 and Stan objects and not Inference Data.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#282 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AG4S4XqzCoIt_DWg3SFvUmEqapcrSGFKks5ueg-vgaJpZM4W33tP>
.
|
|
heads up #283 needs to merge before this one |
canyon289
force-pushed
the
autocorrplot_test
branch
from
f11b986
to
a9cddcd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One question I have,
Does it make sense to have all the plot tests take a pymc3.MultiTrace and a StanFit4Anon model as an input, and instead have az.InferenceData as an input?
I ask because these objects get converted to InferenceData right at the top of most plots, and I vaguely feel like we should have
(pymc3.MultiTrace, StanFit4Anon) -> Inference Databe its own test and decouple the object conversion from "does it plot" test?I could make arguments about repeated computation, and more atomic tests, but if someone has strong(er) feelings let me know.