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

Add information on methods for mcse #16

Merged
merged 5 commits into from
Jul 4, 2021
Merged

Conversation

rikhuijzer
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #16 (ea77f1f) into main (c5125f4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #16   +/-   ##
=======================================
  Coverage   94.39%   94.39%           
=======================================
  Files           8        8           
  Lines         553      553           
=======================================
  Hits          522      522           
  Misses         31       31           
Impacted Files Coverage Δ
src/mcse.jl 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5125f4...ea77f1f. Read the comment docs.

src/mcse.jl Outdated Show resolved Hide resolved
src/mcse.jl Outdated Show resolved Hide resolved
src/mcse.jl Outdated Show resolved Hide resolved
src/mcse.jl Show resolved Hide resolved
@rikhuijzer
Copy link
Contributor Author

Thanks for the feedback, David. Let me know if I can also update the MCMCChains docs. I'm not sure what you want with that exactly. For me, closing that PR is also fine if you plan to merge the migration to InferenceDiagnostics.jl soon(ish).

@rikhuijzer
Copy link
Contributor Author

According to Benjamin Deonovic on Slack. The methods are based on other calculations. Don't merge yet, I'll add refs.

@rikhuijzer
Copy link
Contributor Author

Okay. Updated it. Are the refs okay like this? I see that you also use

**references**

[...]

but that isn't so useful here. Alternatively, we can move the docs in the different methods and add a bunch of [mcse_bm](@ref) etc

@devmotion
Copy link
Member

I think it's fine as it is. IIRC I did not use cross references since I was not satisfied with the Documenter output (e.g., there are issues with references that appear in different docstrings (JuliaDocs/Documenter.jl#566) but maybe this is not an issue anymore since I removed the references from FFTESSMethod).

@devmotion
Copy link
Member

Thanks for the feedback, David. Let me know if I can also update the MCMCChains docs. I'm not sure what you want with that exactly. For me, closing that PR is also fine if you plan to merge the migration to InferenceDiagnostics.jl soon(ish).

I plan to register the package within the next days (#2 (comment)) and there is already a PR for MCMCChains, so I assume the migration can be completed until the end of next week. So maybe we can just wait until end of next week and merge the PR to MCMCChains only if it is not fixed until then?

@rikhuijzer
Copy link
Contributor Author

Thanks for the feedback, David. Let me know if I can also update the MCMCChains docs. I'm not sure what you want with that exactly. For me, closing that PR is also fine if you plan to merge the migration to InferenceDiagnostics.jl soon(ish).

I plan to register the package within the next days (#2 (comment)) and there is already a PR for MCMCChains, so I assume the migration can be completed until the end of next week. So maybe we can just wait until end of next week and merge the PR to MCMCChains only if it is not fixed until then?

Completely fine by me :) I'll just close the other PR for now.

@devmotion devmotion merged commit f9a3eb5 into TuringLang:main Jul 4, 2021
@devmotion
Copy link
Member

Thanks for the PR!

@rikhuijzer rikhuijzer deleted the patch-1 branch July 4, 2021 20:25
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