Skip to content

All values passed for string option values (except for file names) are converted to lower case - #3108

Merged
jajhall merged 2 commits into
latestfrom
fix-3107
Jun 30, 2026
Merged

All values passed for string option values (except for file names) are converted to lower case#3108
jajhall merged 2 commits into
latestfrom
fix-3107

Conversation

@jajhall

@jajhall jajhall commented Jun 28, 2026

Copy link
Copy Markdown
Member

Closes #3107

@odow

odow commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

The counter point to this is that you could accept only the case sensitive strings and error when something doesn't match? (Shouldn't that already be the case?)

@odow

odow commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator
julia> highs = Highs_create()
Ptr{Nothing}(0x0000000818ef0000)

julia> Highs_setStringOptionValue(highs, "parallel", "On")
ERROR:   Value "On" for parallel option is not one of "off", "choose" or "on"
-1

Seems good to me... I wonder why the OP didn't realise in #3106?

@jajhall

jajhall commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

That's what was done before - other than strings for bool options - but today someone used "On", not "on" for the parallel option. He and I both lost time trying to work out what happened.

Of course he didn't check the HighsStatus::kError return 🙄

@jajhall

jajhall commented Jun 29, 2026

Copy link
Copy Markdown
Member Author
julia> highs = Highs_create()
Ptr{Nothing}(0x0000000818ef0000)

julia> Highs_setStringOptionValue(highs, "parallel", "On")
ERROR:   Value "On" for parallel option is not one of "off", "choose" or "on"
-1

Seems good to me... I wonder why the OP didn't realise in #3106?

He's just acknowledged that the error logging is there for him to see!

@galabovaa

galabovaa commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

LGTM, should we do it for all options?

I guess you are missing the "solver" option in the list

-------------------------------------------------------------------------------
string-option-case-insensitivity
-------------------------------------------------------------------------------
/home/ivet/code/HiGHS/check/TestOptions.cpp:564
...............................................................................

/home/ivet/code/HiGHS/check/TestOptions.cpp:574: FAILED:
  REQUIRE( highs.setOptionValue(kSolverString, "HiPO") == HighsStatus::kOk )
with expansion:
  -1 == 0

@jajhall

jajhall commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

LGTM, should we do it for all options?

I guess you are missing the "solver" option in the list

-------------------------------------------------------------------------------
string-option-case-insensitivity
-------------------------------------------------------------------------------
/home/ivet/code/HiGHS/check/TestOptions.cpp:564
...............................................................................

/home/ivet/code/HiGHS/check/TestOptions.cpp:574: FAILED:
  REQUIRE( highs.setOptionValue(kSolverString, "HiPO") == HighsStatus::kOk )
with expansion:
  -1 == 0

Do you get this failure with fix-3107 built locally?

It runs fine on oronsay and fletcher

I saw all the CI test failures, but couldn't see from the logs what was failing

@jajhall

jajhall commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

LGTM, should we do it for all options?

What I've implemented trims all prospective string option values and converts them to lower case for all but the options that are file names

I guess you are missing the "solver" option in the list

Which list? I'm confused

@galabovaa

galabovaa commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

No, I got confused!

I thought the list of options in possibleLowerCaseOptionValue(.. is a set of which options we will convert to lower case, but it is just an exclusion of the file options, as you say.

Yes, I got the failure on my linux laptop! Tested the changes quickly with

cmake -S. -B build -DALL_TESTS=ON
cmake --build build -j30
./build/bin/unit_tests [highs_options]

Possibly deleting the build folder may help you to reproduce

@jajhall

jajhall commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

I was building with -DHIPO=on !

@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.16%. Comparing base (9713e3d) to head (e418d50).

Additional details and impacted files
@@           Coverage Diff           @@
##           latest    #3108   +/-   ##
=======================================
  Coverage   73.15%   73.16%           
=======================================
  Files         430      430           
  Lines      103937   103967   +30     
  Branches    16717    16718    +1     
=======================================
+ Hits        76039    76067   +28     
- Misses      27622    27624    +2     
  Partials      276      276           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jajhall
jajhall merged commit 2f96b88 into latest Jun 30, 2026
504 of 506 checks passed
@jajhall
jajhall deleted the fix-3107 branch June 30, 2026 09:50
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.

3 participants