-
Notifications
You must be signed in to change notification settings - Fork 15
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
Raise exception instead of auto replace bad characters #90
Comments
I should add that I especially think this should change because my understanding is that this behavior is different than what occurs with gurobi. |
@alexlusak thanks for bringing this up. The goal was to, by default, avoid extraneous extra characters that would cause Gurobi to write model files that can't be read back in. From memory we found pandas objects (especially datetime) behaved a little differently in their string representations to plain python which was part of the reason for that choice. I can certainly see the argument for making this completely consistent with gurobipy. That behaviour is to just let the names go through as-is. There is an option for this in gurobipy-pandas, which is to pass Would that option resolve your issue? I would prefer to leave the defaults as-is unless there are a lot of objections. gurobipy-pandas is meant as a higher-level interface so it seems reasonable to do some extra "help" for the user here. |
By the way, I'm curious to know how the naming impacts indexes when merging data? Does your code require joining variables/constraints based on their names? Or did I misunderstand something here? |
This issue arose because there is a section of our code where we extract Pi values from a set of constraints, and when we parse those constraints out into a dataframe, the second column in my previous example with I'm not entirely sure if just setting the |
An alternative could be to at least print out some warning message to the user that the replacement occurred? |
My point is that gurobipy does not throw an error if it detects a space character (at least to my knowledge, if you're seeing different behaviour please let me know). The most it will do is print a warning, and only does that if you write the model out to file format that forbids these characters. Actually for LP format what it will do there is replace illegal characters with underscores in the written LP. For MPS format, it will ignore the user-provided names entirely.
I can understand the inconsistency is annoying and I'll consider changing the defaults here. Just to be clear though, if we make a change it would be to just not do any character replacement (and not to error out). Note though that we may not be able to make the naming fully consistent, since gurobipy generates names typically from an iterable of python objects, while gurobipy-pandas is using a pandas index.
When writing models either with gurobipy-pandas, or with gurobipy directly, I would always avoid having the logic of the program depend on the generated names of individual variables and constraints. This is really making program logic depend on python's Could you please share a minimal example? I'd like to understand the workflow here, I think I must be missing something, sorry. |
IMHO, this is bad practice. When you create your constraints with It is not a good practice to use the names of constraints or variables to do joins back to your original data. |
I have a constraint where the index is something akin to the following:
when adding this constraint directly through Gurobi, this would return an error due to incorrect characters. However, in gurobipy-pandas, I noticed the "bad" characters are automatically replaced with
_
, as in the following:This presents an issue when we go to extract outputs from the model and merge things back on to our original dataframes, since now the index has changed. Either we merge rows out or see NA values when left merging.
I think that this line should be modified, preferably to raise an error instead of just performing the string replace:
I also think it's probably worth it to fix the datetime conditional branch as well - this should really force users to have datetimes represented in
"%Y_%m_%dT%H_%M_%S"
formatting.The text was updated successfully, but these errors were encountered: