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

Bugs in the docs for the nonlinear example #80

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

mattiasvillani
Copy link
Contributor

The changes in PR #79 led to the following bugs (some edits in #79 following the review strangely got lost in the final version):

  • The title got removed from the example doc
  • Long printout of not important vector
  • The replay of the trajectories somehow got killed in edits following the PR preview which led the pgas trajectories not showing in the plot.
  • The rng argument that was requested to be added to the pgas call also got wiped out in the edits from the review.

This PR fixes all four issues.

@mattiasvillani mattiasvillani changed the title Fixed Bugs in the docs for the nonlinear example Sep 18, 2023
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (72a1e55) 96.32% compared to head (138b4c6) 96.32%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   96.32%   96.32%           
=======================================
  Files           7        7           
  Lines         381      381           
=======================================
  Hits          367      367           
  Misses         14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trajectories = map(chains) do sample
replay(sample.trajectory)
end
particles = hcat([trajectory.model.f.X for trajectory in trajectories]...);
Copy link
Member

Choose a reason for hiding this comment

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

This should have been (without using the suggestion tool, since I messed up everything last time):

particles = hcat([trajectory.model.f.X for trajectory in chains]...); # pulling it from the chains directly

and you should be able to remove the replay part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would need to read up on the replay idea here, but replacing

trajectories = map(chains) do sample replay(sample.trajectory) end particles = hcat([trajectory.model.f.X for trajectory in trajectories]...);

with

particles = hcat([trajectory.model.f.X for trajectory in chains]...); # pulling it from the chains directly

gives me the error:

ERROR: type PGSample has no field model

Copy link
Member

Choose a reason for hiding this comment

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

This fixes this issue

particles = hcat([chain.trajectory.model.X for chain in pgas_chains]...)

but seems to hit another one with the way we copy models around. I'll open an issue and look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can keep the (possibly redundant) replay part for now and merge my PR? That would be better than the severely broken version merged at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, since the fix pretty much requires #75 to be merged first

@yebai yebai merged commit 46ed087 into TuringLang:master Oct 5, 2023
@yebai
Copy link
Member

yebai commented Oct 5, 2023

Thanks @mattiasvillani and @FredericWantiez!

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