Skip to content

Serval small fixes that do not affect any existing tests: - #116

Merged
zurabksmlp merged 5 commits into
masterfrom
small_fixes
Apr 29, 2026
Merged

Serval small fixes that do not affect any existing tests:#116
zurabksmlp merged 5 commits into
masterfrom
small_fixes

Conversation

@zurabksmlp

Copy link
Copy Markdown
Collaborator
  1. model refinement mode creates a DOE file that can be used to re-train the model. This file was printed out as _doe.cex while the intension is that it will be used internally. Printing this file is disabled now so after running a regression one will not see this strange-looking file _doe.cex.
  2. Command line options -rad_rel and -rad_abs should override the respective radii values specified in the spec file. This feature was not working and there were no regression tests to test it. Fixed handling of command-line radii and added test 228 and 229 -- the first one tests that overriding the radii works correctly, and the second one tests that every knob has a radius specified.
  3. Fixed an overly restrictive assertion that checks that variables occurring in the model function definitions (when -model system is specified) match features defined in data. Added test 230 to cover this sanity check.

1. model refinement mode creates a DOE file that can be used to re-train the model. This file was printed out as _doe.cex while the intension is that it will be used internally. Printing this file is disabled now so after running a regression one will not see this strange-looking file _doe.cex.
2. Command line options -rad_rel and -rad_abs should override the respective radii values specified in the spec file. This feature was not working and there were no regression tests to test it. Fixed handling of command-line radii and added test 228 and 229 -- the first one tests that overriding the radii works correctly, and the second one tests that every knob has a radius specified.
3. Fixed an overly restrictive assertion that checks that variables occurring in the model function definitions (when -model system is specified) match features defined in data. Added test 230 to cover this sanity check.
@zurabksmlp
zurabksmlp requested a review from fbrausse April 27, 2026 12:49

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Regression in Ubuntu 24.04 virtual environment finished with one diff

> =================== Diff report for: Test230_smlp_toy_monotone_basic_trace.csv ==================================
> 6c6
> < ca,sat,1/20,0,0,0,0
> ---
> > ca,sat,1/20,1,0,1,0
> =================== End of Test230_smlp_toy_monotone_basic_trace.csv diff report ================================

Tag: v1.2.3rc1
Installation:

pip install -i https://test.pypi.org/simple --extra-index-url https://pypi.org/simple smlptech==1.2.3rc1

Apparently expected results should be fixed or code should be changed

@zurabksmlp

Copy link
Copy Markdown
Collaborator Author

Many thanks Dmitry for running regression. This diff is due to Z3 version, as I am not using 4.8.12. You only see difference in regression results only in a new test because I have not updated masters of any existing test. Also, the diff confirms that the result has not change and only one call to SMT solver returned different SAT assignment. It is safe to update master for this debug trace file.

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. Updated master/Test230_smlp_toy_monotone_basic_trace.csv after talking to Zurab.
    One line diff is probably explained by difference in z3 version used.
  2. Need to investigate graphic window opening during the run.
  3. Checked that test works fine with virtual display

@mdmitry1
mdmitry1 self-requested a review April 28, 2026 08:17

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approval pending investigation of unexpected graphic window opening during Test230 run

zurabksmlp added 2 commits April 29, 2026 00:57
…del system option value is used; in such a case we do not train a modesl and do not use data and the plots are not relevant.
@zurabksmlp

Copy link
Copy Markdown
Collaborator Author

I have investigated the issue "unexpected graphic window opening during Test230 run". The fix was to disable plots when -model system option value is used since in this case we do not train a model and do not use data so plots are not relevant. I have pushed the changes to origin, small_fixes branch.

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Regression in Ubuntu 24.04 virtual environment finished with one diff:
=================== Diff report for: Test230_smlp_toy_monotone_basic_resp-distr.png ==================================

diff: /home/mdmitry/github/smlp_small_fixes/scripts/venv/smlp_package_venv/smlp/regr_smlp/code/Test230_smlp_toy_monotone_basic_resp-distr.png: No such file or directory
=================== End of Test230_smlp_toy_monotone_basic_resp-distr.png diff report ================================
Tag: v1.2.3rc2
Fix should be: removing Test230_smlp_toy_monotone_basic_resp-distr.png from master

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Redundant file is removed
Approved

commit fe0a1fb7bcc3b68ed2977e2a2259d8a42834488e (HEAD -> small_fixes, origin/smlp_small_fixes_test, origin/small_fixes, smlp_small_fixes_test)
Author: Dmitry Messerman <dmitry.messerman@gmail.com>
Date:   Wed Apr 29 09:45:20 2026 +0300

    Removing redundant png file for Test230

@mdmitry1
mdmitry1 self-requested a review April 29, 2026 06:50

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved

@zurabksmlp
zurabksmlp requested review from mdmitry1 April 29, 2026 07:43

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved

@mdmitry1 mdmitry1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved

@mdmitry1

Copy link
Copy Markdown
Collaborator

@fbrausse you approval is required, as I've made two commits

@fbrausse fbrausse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. Just a comment on a minor thing regarding Python's in on dictionaries.

Comment thread src/smlp_py/smlp_spec.py
self._SPEC_KNOBS_RELATIVE_RADIUS not in var_spec.keys():
raise Exception('Neither absolute nor relative radius is specified for variable ' + str(var_spec))
if self._SPEC_KNOBS_ABSOLUTE_RADIUS in var_spec.keys() and \
self._SPEC_KNOBS_RELATIVE_RADIUS in var_spec.keys():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need for .keys() here.

@zurabksmlp
zurabksmlp merged commit 0b7e036 into master Apr 29, 2026
@zurabksmlp
zurabksmlp deleted the small_fixes branch April 29, 2026 14:43
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