Skip to content

scipy minimize generator - #424

Open
roussel-ryan wants to merge 29 commits into
mainfrom
bobyqua
Open

scipy minimize generator#424
roussel-ryan wants to merge 29 commits into
mainfrom
bobyqua

Conversation

@roussel-ryan

@roussel-ryan roussel-ryan commented May 27, 2026

Copy link
Copy Markdown
Collaborator

This pull request introduces the new ScipyGenerator, a generic wrapper for scipy.optimize.minimize, and integrates it into the Xopt sequential optimization framework. The documentation and example notebooks are updated to reflect this addition, and some generator imports and references are reorganized for clarity and consistency.

Integration model

Xopt uses ask/tell semantics (one new point per step), while scipy minimize expects direct access to objective evaluations. This class bridges the two by running one persistent minimize call in a worker thread:

  1. Build a cache from existing Xopt observations.
  2. Start scipy.optimize.minimize once with an objective function that first checks the cache.
  3. If scipy asks for an unseen point, send that point to Xopt and block until Xopt provides the external objective value.
  4. Continue the same scipy run from in-memory state until convergence.

Performance implications

  • _build_cache is O(N) in number of past evaluations and is executed when the persistent session starts.
  • The active scipy run is maintained in memory between step calls.
  • Point keys are rounded (12 decimals) before cache lookup to avoid fragile floating-point equality checks.

Key changes:

1. ScipyGenerator Integration

  • Added ScipyGenerator as a new sequential generator, providing a generic interface to scipy.optimize.minimize methods within Xopt's ask/tell workflow. This includes registration in generator discovery and import paths. [1] [2] [3]

2. Documentation Enhancements

  • Created a new API documentation page (docs/api/generators/sequential/scipy.md) describing ScipyGenerator, its integration model, configuration, and performance considerations.
  • Updated the algorithms overview (docs/algorithms.md) to list ScipyGenerator and clarify the location of the LatinHypercubeGenerator example.
  • Added ScipyGenerator to the main feature list in the project index.
  • Updated navigation in mkdocs.yml to include the new Scipy Minimize example and API documentation, and moved the Latin Hypercube example to the "Other" section. [1] [2]

3. Example Notebooks

  • Added a new example notebook (docs/examples/sequential/scipy.ipynb) demonstrating the use of ScipyGenerator with the Rosenbrock function.
  • Moved and updated the Latin Hypercube example notebook to a new location, reflecting its new import path.

4. Generator Import and Registration Cleanup

  • Removed redundant imports from xopt/generators/scipy/__init__.py and updated import paths to reflect the new organization.
  • Improved the generator defaults logic to handle fields with a default_factory in xopt/generators/__init__.py.

These changes collectively add a powerful new optimization capability to Xopt and improve the clarity and usability of the documentation and generator registration.

@codecov

codecov Bot commented May 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.99499% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
xopt/generators/sequential/scipy.py 95.83% 1 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

@roussel-ryan

Copy link
Copy Markdown
Collaborator Author

@copilot address the coverage, linting and notebook failures

Copilot AI commented May 27, 2026

Copy link
Copy Markdown
Contributor

@copilot address the coverage, linting and notebook failures

Fixed in commits 42f6071 and 57bde15 (approximately):

  • Tests (test_serialization_loading[scipy]): get_generator_defaults now handles Pydantic v2 default_factory fields (which set v.default to PydanticUndefined instead of None), resolving the TypeError: Object of type PydanticUndefinedType is not JSON serializable.
  • Lint (ruff-format): Fixed x0 ** 2x0**2 in the scipy example notebook.
  • Lint (end-of-file-fixer): Removed the test artifact .txt files (all_sequential_tests.txt, test_results.txt, test_serialization_results.txt) from git tracking and added them to .gitignore.

@roussel-ryan

Copy link
Copy Markdown
Collaborator Author

@copilot add tests to achieve needed coverage

Copilot AI commented May 27, 2026

Copy link
Copy Markdown
Contributor

@copilot add tests to achieve needed coverage

Added 8 new tests in the latest commit covering all previously untested branches in scipy.pyscipy.py now has 100% line coverage:

  • Validator rejecting whitespace-only method string
  • Validator rejecting empty initial_point dict
  • _add_data early return on empty DataFrame
  • Unknown solver ValueErrorRuntimeError conversion
  • Non-solver ValueError re-raise
  • Convergence path (minimize finishes without requesting a new point)
  • Convergence path with constants
  • Candidate generation with constants

Also removed a dead-code if v is None: return v branch that was unreachable in Pydantic v2.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds a new sequential optimization generator, ScipyGenerator, that adapts scipy.optimize.minimize to Xopt’s external ask/tell workflow, and updates tests, docs, and generator registration to support it.

Changes:

  • Introduce ScipyGenerator (sequential wrapper around scipy.optimize.minimize) and register it under generator name scipy.
  • Add unit tests (including serialization/restart coverage) for the new generator and update existing sequential serialization tests.
  • Add documentation + example notebook for ScipyGenerator, and update MkDocs navigation and algorithms/index references; refactor LatinHypercubeGenerator imports/location.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
xopt/generators/sequential/scipy.py New ScipyGenerator implementation wrapping scipy.optimize.minimize in a sequential ask/tell pattern
xopt/generators/sequential/init.py Export ScipyGenerator from the sequential generators package
xopt/generators/init.py Register ScipyGenerator for dynamic loading; improve defaults handling for default_factory fields
xopt/generators/scipy/init.py Removes old scipy generator re-exports (package content removed)
xopt/generators/latin_hypercube.py Adds/relocates LatinHypercubeGenerator under a new import path
xopt/tests/generators/sequential/test_scipy.py Adds test coverage for ScipyGenerator
xopt/tests/generators/sequential/test_serialization.py Adds ScipyGenerator to sequential generator serialization/restart tests
xopt/tests/generators/test_latin_hypercube.py Updates import path for LatinHypercubeGenerator
docs/api/generators/sequential/scipy.md API documentation page for ScipyGenerator
docs/examples/sequential/scipy.ipynb New example notebook demonstrating ScipyGenerator usage
docs/examples/other/latin_hypercube.ipynb Updates import path for LatinHypercubeGenerator and relocates example
mkdocs.yml Adds new docs + example notebook to navigation; updates Latin Hypercube example path
docs/index.md Mentions the new scipy sequential generator in the feature list
docs/algorithms.md Adds ScipyGenerator to algorithms list (but link maintenance needed)
.gitignore Ignores local test output artifacts

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/algorithms.md Outdated
Comment thread xopt/generators/sequential/scipy.py Outdated
Comment thread xopt/generators/sequential/scipy.py
@nikitakuklev

Copy link
Copy Markdown
Collaborator

FYI I'll try catch up this week

roussel-ryan and others added 6 commits June 2, 2026 10:57
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@electronsandstuff electronsandstuff 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.

OK, overall I think it's a cool concept. Obviously it's a bit of a hack to get it to work, but it's kind of cool. Because of that, I would strongly recommend adding the below mentioned test. I can't think of how to do it outside of this without threading. There are also obvious performance issues, but it might be good enough for now.

"source": [
"# ScipyGenerator with scipy.optimize.minimize\n",
"\n",
"This notebook demonstrates how to use Xopt's `ScipyGenerator` to drive any supported scipy `optimize.minimize` method in a sequential ask/tell workflow."

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.

"ask/tell" isn't jargon we use anywhere else. This feels like an LLM'ism making up terms. I would recommend using the names used elsewhere for the generator. Check your PR for other uses of this phrase.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

removed

Comment thread docs/examples/sequential/scipy.ipynb Outdated
"source": [
"## Notes on performance\n",
"\n",
"`ScipyGenerator` bridges scipy's in-process `minimize` API into Xopt's external evaluation loop by replaying cached points each step. This adds overhead that is small for expensive evaluations but can be noticeable for very fast toy objectives."

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.

Style thing: I don't know if I'd add low level details in the example

Comment thread .gitignore Outdated

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.

Formatting, make titles title case. Probably don't need a header for imports.

Comment thread xopt/generators/sequential/scipy.py Outdated
cached points. This is robust and method-agnostic, but adds repeated optimizer
bookkeeping work compared to a persistent in-memory scipy run.
- Point keys are rounded (12 decimals) before cache lookup to avoid fragile
floating-point equality checks. This improves replay stability across methods

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.

I am a little worried about dealing with "numerically close" points like this. My understanding is that the scipy optimizers are deterministic, so the hack to get it to work in Xopt is you feed it back the same points and it will always revisit the same sequence seen before and you replay it the objective value. I am worried about trying to do things where we feed it points "nearby" without carefully thinking about how this affects the algorithm.

If the cache is really just replaying points, I would imagine it can be a list, not a dict right? Ie the list of points in the order they have been seen by the optimizer, then we just replay without hashing the variables in a dict. This should work under the same assumptions as in the current version of the code and also be significantly faster (no computation during replay.

Comment thread xopt/generators/sequential/scipy.py

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.

Given that we are doing something not supported by scipy, I think this PR needs the following test:

Given a test objective function, perform an optimization once with scipy.minimize and once with Xopt.ScipyGenerator. Collect all of the x points and confirm all of them are the same down to numerical precision. Parameterize the test by method and do for each of the methods we are supporting in scipy (presumably all that support bounds).

This will ensure that the replay method is doing what we think it should even if scipy changes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

tests revealed issues with this, so thanks for the suggestion! I have moved the implementation to a threaded one such that it exactly reproduces the output from scipy minimize

Comment thread xopt/generators/latin_hypercube.py
@roussel-ryan

Copy link
Copy Markdown
Collaborator Author

Thanks for the comments @electronsandstuff, I will work on this over the next couple of weeks and ping you again when its ready

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.

Comment thread docs/api/generators/sequential/scipy.md Outdated
Comment thread docs/examples/sequential/scipy.ipynb Outdated
Comment thread xopt/tests/generators/sequential/test_serialization.py Outdated
Comment thread xopt/generators/sequential/scipy.py Outdated
roussel-ryan and others added 5 commits July 15, 2026 13:47
@nikitakuklev

nikitakuklev commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

This implementation has a few issues, but overall idea is pretty elegant and worth to add. My big concern is this PR breaks some workflows in unexpected ways. That should be fixed.

I'll just paste the LLM comments I agree with, critical ones in bold. Up to you how much to address.

  • Zombie worker can poison a new session (_stop_session, scipy.py:305). join(timeout=2.0) can expire if the worker is inside scipy numeric code for >2s. The method then replaces _stop_event and both queues on self. The abandoned worker looks these up dynamically via self., so it sees the fresh, unset event, never stops, and will push its stale request into the new session's request queue and consume the new session's responses.
    Fix: pass per-session primitives (event + both queues) into _run_session as arguments so a worker only ever sees its own session's objects, and/or compare a session-generation token.

  • Response queue can be silently offset by one (_add_data, scipy.py:187). Any time the worker is alive, _add_data unconditionally puts to _response_queue. The base class legally allows calling add_data twice with the same matching candidate row — the second put sits in the queue and gets consumed as the answer to the worker's next request, permanently shifting every subsequent response by one. The optimization then goes silently wrong.
    Suggested fix: track an explicit "pending request" (set when _generate dequeues a request, cleared when answered) and respond exactly once.
    Bonus: keying the cache by the requested x rather than the observed data row also protects replay when readback values differ from setpoints within validate_point's rtol=1e-6 — currently that mismatch breaks post-restart cache hits.

  • Post-convergence behavior is wasteful and silent (_start_session_if_needed + _generate, scipy.py:290, 319). Once scipy converges, every later generate() spawns a new thread, rebuilds the cache (O(N)), replays the entire trajectory, converges again, and returns a duplicate of the last data row — which gets re-evaluated and appended. The new notebook's for _ in range(200): X.step() will do exactly this after convergence. Suggest latching a "converged" flag to skip the restart, emitting a warning once on the fallback path, and keeping the scipy OptimizeResult (currently discarded — success/message/nit are lost with no way for the user to detect convergence).

  • getstate mutates the object being pickled (scipy.py:126): it calls _stop_session(), so pickle.dumps(gen) kills the live session on the original. Replay makes this recoverable, but it's a surprising side effect of serialization — at minimum document it; ideally snapshot without stopping.

  • deepcopy yields an inconsistent copy of an active generator (scipy.py:117): is_active=True survives model_dump() but _last_candidate/_data_set don't, so copy.add_data(...) raises SeqGeneratorError until the next generate().

  • The pickle assertion was deleted from test_serialization.py (pickle.dumps(X2.generator)), removing coverage for all four sequential generators — in the same PR that adds getstate/setstate specifically to make ScipyGenerator picklable.
    NK comment: pickling works fine, just test needs to be reenabled

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.

5 participants