-
Notifications
You must be signed in to change notification settings - Fork 1
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
Remove / rename model_file_name in StanCompileRequest #13
Comments
I saw that but kept it in just to stick with monkeying the Stan C++ API. I think it's worthwhile to not make these changes at the interface wrapper level so I disagree with dropping it. Renaming it to match the Stan C++ API argument name makes sense. I should probably stick to that in the proto files. |
Sounds good. I think we also should express the default value then. I gather this isn't done explicitly in protobuf 3, https://developers.google.com/protocol-buffers/docs/proto3#default So I guess we check if |
Yes, protobuf is optimized for transmission so with optional fields the default values are encoded in the interfaces rather than sent on the wire. The only user-configurable default is ENUMs where the first option (0) is taken as the default and you can give it a meaningful value... of course in that case you can't tell if it's the default value because it was set or because it was not set so it better really be a good default meaningful value. |
Closing in favor of #19 |
model_file_name is not used in Stan. It's an artifact of old code. See Stan stan-dev/stan#1752 Closes #16 and #13.
model_file_name is not used in Stan. It's an artifact of old code. See Stan stan-dev/stan#1752 Closes #16 and #13.
So the file name argument in stan::lang::compile (
in_file_name
) is purely aesthetic -- it's used in the error messages, as far as I can tell.I think we should drop it or rename the field (to in_file_name?) in StanCompileRequest. Right now I think it would be very easy to think that it's the location of a source file where the model code is stored.
Thoughts @sakrejda?
The text was updated successfully, but these errors were encountered: