Don't change the casing of model parameters. #251
Conversation
| @@ -1648,7 +1648,6 @@ CLParser::ParseCommandLine(int argc, char** argv) | |||
| "<name:value:type>."); | |||
| } | |||
|
|
|||
| std::for_each(values.begin(), values.end(), ToLowerCase); | |||
| std::string name{values[0]}; | |||
| std::string value{values[1]}; | |||
| std::string type{values[2]}; | |||
There was a problem hiding this comment.
It seems like we used to string match the type value but that part has been removed from the command_line_parser.cc and moved to other places in client repo so I think the lowercasing is still needed. But I think we only need to lowercase the type value (not exactly sure why I was applying it to other ones as well):
| std::string type{values[2]}; | |
| std::string type{values[2]}; | |
| ToLowerCase(type); // to string-match the type |
There was a problem hiding this comment.
After giving it some more thoughts, I think another main issue here is that the place where the lowercasing is actually being used is now very far away. So I'm leaning into just deleting the lowercase entirely here and adding it to the client repo just before the string matching. @nicolasnoble any thoughts?
…meter string. Co-authored-by: Hyunjae Woo <[email protected]>
|
This is the type of change that scares me. |
cc @debermudez
I have very low confidence we won't break anything with this change. @nv-hwoo do you remember why this feature from 2 years ago needed to have lower case parameters?