Skip to content
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

Parsing objective sense in MPS file #656

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stephenjmaher
Copy link

Fixes #655

This PR makes it possible to set the objective sense in an MPS file. Currently, the objective sense in the MPS file is ignored. The sense needs to be supplied to fromMPS as a parameter.

This change does the following:

  • the sense parameter in LpProblem.fromMPS now defaults to None.
  • the readMPS methods checks for the header OBJSENSE in the MPS file. If the header is encounter there are two options:
OBJSENSE   MAX

or

OBJSENSE
   MAX

both options are handled in the readMPS method.

  • If no objective sense is specified in the MPS file, then sense defaults to LpMinimize.
  • If an objective sense is passed to LpProblem.fromMPS, then this overrides the sense that is specified in the MPS file.

@CLAassistant
Copy link

CLAassistant commented May 31, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Stephen J. Maher seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Stephen J. Maher seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I'm still unsure if we should where we should check for None and where to assign the mode of the line. I've left some comments.

Comment on lines +91 to +92
parameters['sense'] = const.LpMaximize if line[1] == 'MAX'\
else const.LpMinimize
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you missing a mode=CORE_FILE_OBJSENSE_MODE here too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this case. This line is catching the case where you have the objective sense provided in the same line as the keyword OBJSENSE, such as

OBJSENSE MAX

In this case we don't need to enter the OBJSENSE mode.

Comment on lines +93 to +94
else:
mode = CORE_FILE_OBJSENSE_MODE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why would we have the same mode regardless of the if clause. Maybe we should put the assignment before the len(line)>1 and take out the else clause? Or maybe I don't understand the idea.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following on from the previous comment, we have to enter the CORE_FILE_OBJSENSE_MODE is the objective sense is not given in the same line as the OBJSENSE keyword. This is catching the case

OBJSENSE
  MAX

The check of len(line) > 1 differentiates between these two cases.

Comment on lines +154 to +156
elif mode == CORE_FILE_OBJSENSE_MODE:
parameters['sense'] = const.LpMaximize if line[0] == 'MAX'\
else const.LpMinimize
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we test if parameters['sense'] is None? or if we provided an argument to the function (i.e., via the sense argument?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put in a check here for whether parameters['sense'] is None. However, in the current code the check is not necessary since it is only possible to enter the CORE_FILE_OBJSENSE_MODE mode if parameters['sense'] is None.

We could put in an assert or throw an exception to protect against future code changes? Something like (after line 154)

if parameters['sense'] is not None
    raise Exception("The supplied objective sense will be overwritten by the MPS file objective sense")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reading MPS file doesn't parse the objective sense
3 participants