Skip to content

mosek_direct.py : Fixed iparam parsing and added support for Task.putparam #3488

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Utkarsh-Detha
Copy link
Contributor

Fixes:

In _apply_solver

  • Call to putintparam was missing in the isinstance(option, str) conditional when parsing iparam. Fixed.
  • Was calling option.pop('mosek') instead of option.pop(0) within the same conditional. Fixed.

Summary/Motivation:

A user wanted to be able to pass the value of ALL parameters as strings. MOSEK's optimizer API supports this through the Task.putparam function, which accepts a parameter name (string) and a value (also string).

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Utkarsh-Detha and others added 2 commits February 24, 2025 14:52
…ameter and the parameter value as strings, as long as the "generic" parameter names are used.

Fixed: call to putintparam was missing in the isinstance(option, str) conditional when parsing 'iparam'.
@mrmundt
Copy link
Contributor

mrmundt commented Mar 3, 2025

@Utkarsh-Detha - please run black -S -C on your branch (with the newest version of black)

@Utkarsh-Detha
Copy link
Contributor Author

Thank you for the review, @mrmundt . I ran the formatter as you recommended, with the newest version thereof.

@blnicho
Copy link
Member

blnicho commented Mar 5, 2025

@Utkarsh-Detha could you add a couple tests for these changes?

@mrmundt
Copy link
Contributor

mrmundt commented Apr 8, 2025

@Utkarsh-Detha - Pinging again about this! Could you please add in some tests?

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

Overall this looks reasonable, but could be improved by some defensive checks. In addition, it would be good to test this functionality thought the unit tests.

self._solver_model.putstrparam(param, option)
elif 'dparam' in key.split('.'):
self._solver_model.putdouparam(param, option)
elif 'iparam' in key.split('.'):
Copy link
Member

Choose a reason for hiding this comment

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

I worry that there is no else: clause: users who forget to explicitly include .sparam, .dparam, or .iparam will have those options silently ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the if clause, I call param = getattr(mosek, param[0])(param[1]). If an invalid key is in the options dict, it should result in an AttributeError. So I thought it should be ok even without an else statement.

elif 'iparam' in key.split('.'):
if isinstance(option, int):
self._solver_model.putintparam(param, option)
elif isinstance(option, str):
Copy link
Member

Choose a reason for hiding this comment

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

Same here - without an else, option values that aren't int / str will be silently skipped (this would include, e.g., numpy ints)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have included a warning under an else statement in this situation.

1. Included warning for the case when an invalid input value is passed for iparams.
2. Added unit-test to check parameter-setting functionality in mosek_direct.
@Utkarsh-Detha
Copy link
Contributor Author

@blnicho @mrmundt sorry for the delay in getting this done. I have now added a few unit tests to check the parameter adding functionality. Feel free to suggest any corrections/changes. Thank you for your help.

@Utkarsh-Detha
Copy link
Contributor Author

@jsiirola I am starting to wonder if I should simply remove the option of allowing strings as iparam values. The idea initially was to allow users to pass MOSEK enums as strings, for example mosek.onoffkey.off. But I now think that it is more straightforward to just pass the option value directly to MOSEK, which would raise a TypeError if wrong input type is provided. As this would mean removing functionality, I wanted your opinion. (Lmk if I should rather open an Issue to discuss this)

2. Added a test to check if an AttributeError is raised when wrong parameter names are passed in options.
@mrmundt mrmundt requested review from mrmundt and jsiirola April 15, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

5 participants