search

sakrejda / protostan

Thin protobuf interface wrapper for Stan
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Remove / rename model_file_name in StanCompileRequest #13

Closed ariddell closed 8 years ago

ariddell commented 8 years ago

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.

     * @param in_file_name Name of input file to use in error
     * messages; defaults to <code>input</code>.

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?

sakrejda commented 8 years ago

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.

ariddell commented 8 years ago

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 in_file_name (the renamed model_file_name) is the empty string and, if so, make it "input".

sakrejda commented 8 years ago

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.

ariddell commented 8 years ago

Closing in favor of #19