StanJulia / CmdStan.jl

CmdStan.jl v6 provides an alternative, older Julia wrapper to Stan's `cmdstan` executable. CmdStan will be deprecated in 2022.
MIT License
30 stars 12 forks source link

model type interpreted as Cmdstan.Variational in read_samples function #96

Closed mkshirazi closed 4 years ago

mkshirazi commented 4 years ago

https://github.com/StanJulia/CmdStan.jl/blob/master/src/utilities/read_samples.jl#L51

I am using a package which uses CmdStan for generating samples. If I just call the package and not CmdStan, the ftype determined in line 41 is cmdstan.variational. The code is as follows: ftype = lowercase(string(typeof(m.method)))

This causes a problem in Line 51 (linked above) since the generated CSV file is of the form modelname_variational_1.csv and not modelname_cmdstan.variational_1.csv.

A possible solution to this would be to identify if cmdstan is prepended to variational in method type and then remove it. I have implemented on my local machine and this works fine.

goedman commented 4 years ago

Has the package you are using been published? I'm just looking for a MWE of a case where this is happening.

I think I've seen this issue a while ago, quite a while ago in fact (when the scope became bound to packages). How did you solve it?

mkshirazi commented 4 years ago

Unfortunately, the package has not been published. The simple solution was to call using CmdStan every single time.

The more convenient solution was to change the substring ftype to trim "cmdstan" if it was prepended in the read_samples package.

I can make a PR soon to review if you would like that.

goedman commented 4 years ago

That would be great, I’ll wait for a PR!

mkshirazi commented 4 years ago

I have created a MWE package for this issue here https://github.com/mkshirazi/JuliaTestPackage . In the given package, simply running this would suffice :

using JuliaTestPackage

main_run()

I have also made a PR that provides an easy fix for this particular problem.

goedman commented 4 years ago

Hi @mkshirazi

Thanks for the MWE and the PR. I merged the PR, will release later today or tomorrow. The MWE was also useful and conformed what I expected (qualification of method names when included in a package).

Let me know if this doesn't solve your issue, thanks again for your help, Rob