Bring the fixture's method entries in line with opm-simulators - #35
Open
hakonhagland wants to merge 1 commit into
Open
hakonhagland wants to merge 1 commit into
hakonhagland wants to merge 1 commit into
Conversation
tests/files/docstrings_simulators.json is a snapshot of the docstring file from opm-simulators, last refreshed in January 2025. Four of its method signatures still use names that the bindings no longer have: mpi_init now setup_mpi get_fluid_state_variable now get_fluidstate_variable get_primary_var_meaning now get_primary_variable_meaning get_primary_var_meaning_map now get_primary_variable_meaning_map OPM/opm-simulators#7439 corrected these in the real file. The fixture kept them, and a review of OPM#31 took the fixture as the definition of the API and suggested replacing a working setup_mpi() call with mpi_init(), which does not exist. Also carries over the two other changes the real file has since had: the setupMpi docstring's inline literal no longer has a third backtick, and checkSimulationFinished documents its return type. All 17 method entries are now identical to the real file on opm-simulators master, with {{name}} expanded to BlackOilSimulator. No test depends on the fixture's contents; conftest.py only copies it into a temporary repository.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The fixture changes are focused, verified, and contain no blocking issues.
Review effort: Lite
Findings: None
What changed in this PR
Updates the simulator docstring fixture to match current upstream method names and documentation.
Changes:
- Corrects four stale method signatures.
- Fixes MPI literal formatting.
- Adds the
checkSimulationFinishedreturn type.
| File | Description |
|---|---|
python/sphinx_docs/tests/files/docstrings_simulators.json |
Synchronizes simulator method entries with upstream bindings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
python/sphinx_docs/tests/files/docstrings_simulators.jsonis a copy of the docstring file from opm-simulators, last refreshed in January 2025. Four of its method signatures still use names that the bindings no longer have. No test depends on the fixture's contents, but anything that searches this repository for the Python API finds it. An automated review of #31 did exactly that. It relied on this file and suggested replacing a workingsim.setup_mpi(...)call withsim.mpi_init(...), which does not exist.Bring the fixture's method entries in line with opm-simulators (commit 1)
mpi_init→setup_mpiget_fluid_state_variable→get_fluidstate_variableget_primary_var_meaning→get_primary_variable_meaningget_primary_var_meaning_map→get_primary_variable_meaning_mapsetupMpidocstring's inline literal no longer has a stray third backtick, andcheckSimulationFinishednow documents its return type.Verification
python/docstrings_simulators.jsonon opm-simulators master, with{{name}}expanded toBlackOilSimulator. All 17 method entries are now identical in both signature and docstring; before this change 13 signatures matched.conftest.py, andtest_view_docs.pychecks only the URL that gets opened, so no test reads its contents.Not included: the constructors
The fixture documents the two constructors as
__init__(deck_filename: str)and__init__(deck, state, schedule, summary_config). That matches the real file in opm-simulators, but the bindings name these arguments differently:filename, andDeck,EclipseState,Schedule,SummaryConfig, plus an optionalargsthat is not documented at all. Passing the arguments by keyword as currently documented raisesTypeError, while passing them by position works, which is what the existing examples do.That needs fixing in opm-simulators first, since the fixture should stay a copy of the real file. I will open that as a follow-up to OPM/opm-simulators#7439 and then refresh the fixture's constructor entries to match, either here or in a small follow-up PR, depending on which merges first.