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

Metropolis Hastings, Legacy version #136

Closed
smjanke opened this issue Oct 7, 2021 · 5 comments · Fixed by #325
Closed

Metropolis Hastings, Legacy version #136

smjanke opened this issue Oct 7, 2021 · 5 comments · Fixed by #325

Comments

@smjanke
Copy link
Contributor

smjanke commented Oct 7, 2021

https://github.com/NQCD/NonadiabaticMolecularDynamics.jl/blob/f194f7380535af288303c15967ac2312bd6e85e9/docs/src/initialconditions/samplingmethods/metropolishastings.md#L29

Why are you describing the legacy version if you plan to eliminate it, anyway? Does it offer any advantages or other features than the current version? In case it doesn't do the latter and really has no reason why anyone would use it over the AdvanceHM.jl, I would not mention it in the documentation, since you're planing to delete it, anyway. You don't want anyone to start using it.

It is very nicely explained section in terms of parameters and output.

@jamesgardner1421
Copy link
Member

Yeah I got myself into a bit of an awkward situation here by having both. Currently this one allows for some stuff that the newer version doesn't and I haven't gotten around to porting everything over just yet. So I'm not sure I'll be deleting it any time soon and thought it best to include just in case. Though hopefully at some point I'll improve the new version enough so that it has all the features of the old version and can delete it then.

I'll leave the issue open to remind us to address this in the future.

@smjanke
Copy link
Contributor Author

smjanke commented Oct 13, 2021

In that case, I think I wouldn't mention that you plan to eliminate it.

Good idea to leave it open.

@reinimaurer1
Copy link
Member

The docs should only mention the new version, but we don't have to delete the old one

@Alexsp32
Copy link
Member

Alexsp32 commented Nov 6, 2023

Maybe update docs

@Alexsp32
Copy link
Member

#325 removes the legacy method from the docs since everything should be covered in some way by the interface to AdvancedMH.jl

@Alexsp32 Alexsp32 linked a pull request Jan 25, 2024 that will close this issue
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 a pull request may close this issue.

4 participants