Use pybind11 for smspec_node by eivindjahren · Pull Request #1258 · equinor/resdata

eivindjahren · 2026-07-23T12:57:53Z

This fixes a bug with smspec_node::valid_type where a nullptr check was done too late. It also makes the operators of ResdataSMSPECNode follow normal conventions for unimplemented types.

Copilot

Pull request overview

This PR migrates smspec_node to a pybind11-based Python binding, fixes a null-pointer handling bug in smspec_node::valid_type, and updates both C++ and Python call sites/tests to use the modernized API and Python comparison semantics.

Changes:

Introduces a new pybind11 module for ResdataSMSPECNode and removes the legacy Python cwrap wrapper.
Fixes smspec_node::valid_type to ensure nullptr/dummy-well checks are performed safely and consistently.
Updates C++ call sites away from the removed “old API” free functions and adds targeted unit/property tests in both C++ and Python.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/rd_tests/test_rd_sum.py	Adds Hypothesis + unit tests for `ResdataSMSPECNode` hashing/comparison semantics and `Summary.writer().add_variable()` behavior.
python/resdata/summary/rd_sum.py	Switches `add_variable()`/`smspec_node()` to return the pybind11 `ResdataSMSPECNode` directly (no `createCReference`).
python/resdata/summary/rd_smspec_node.py	Removes the legacy cwrap-based `ResdataSMSPECNode` implementation.
lib/tests/test_rd_sum.cpp	Updates tests to use `rd::smspec_node` member accessors instead of removed free functions.
lib/resdata/tests/test_smspec_node.cpp	Adds C++ tests for `smspec_node` ordering, totals/rates identification, and default NUMS behavior.
lib/resdata/smspec_node.cpp	Fixes `valid_type()` ordering for safe dummy-well and nullptr handling; removes legacy free-function wrappers.
lib/resdata/smspec_node_pybind.cpp	Adds the new pybind11 module exporting `ResdataSMSPECNode` into Python.
lib/resdata/rd_sum.cpp	Updates internal callers to use `rd::smspec_node` methods (unit/key/params_index).
lib/resdata/rd_sum_vector.cpp	Updates vector building logic to use `rd::smspec_node` methods.
lib/resdata/rd_sum_tstep.cpp	Updates timestep node indexing to use `rd::smspec_node::get_params_index()`.
lib/resdata/rd_sum_pybind.cpp	Changes `_get_var_node` / `_add_variable` / `_add_local_variable` to return `rd::smspec_node*` directly with `keep_alive`.
lib/resdata/rd_sum_data.cpp	Updates interpolation/indexing logic to use `rd::smspec_node` methods (`is_rate`, `get_params_index`).
lib/resdata/rd_smspec.cpp	Updates SMSPEC IO/metadata code to use `rd::smspec_node` accessors.
lib/include/resdata/smspec_node.hpp	Removes the legacy free-function API declarations (partially) and adds C++ comparison operators.
lib/CMakeLists.txt	Builds the new `rd_smspec_node` pybind module and renames/adds the C++ smspec node test executable.
applications/resdata/sum_write.cpp	Updates documentation comments to reference `rd::smspec_node` methods instead of removed free functions.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node.cpp:1353

smspec_node_cmp() is still declared in the public header (lib/include/resdata/smspec_node.hpp), but its implementation was removed from this translation unit. That will cause unresolved-symbol link failures for any downstream code still using this API. Either reintroduce the wrapper definition (preferred for compatibility) or remove the declaration from the header if the C-style API is being retired.

} // end namespace rd

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

eivindjahren · 2026-07-23T13:53:37Z

+            "\n"
+            "   sum.smspec_node(\"FOPT\").num     => None\n"
+            "   sum.smspec_node(\"BPR:1000\").num => 1000\n"


This is not the case, but there is this weird SMSPECS_NUMS_INVALID check. We should not change this behavior, but it could return e.g. 0 if the internal field is not SMSPECS_NUMS_INVALID which the user could create through add_variable

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

lib/resdata/smspec_node_pybind.cpp:111

The docstring examples for num claim sum.smspec_node("FOPT").num => None, but the current binding returns an integer whenever need_nums() is true (and the new tests expect 0 for field variables). Please update the example to match the actual behavior to avoid confusing users.

            "Many of the summary keywords have an integer stored in the\n"
            "vector NUMS as an attribute, i.e. the block properties have\n"
            "the global index of the cell in the nums vector. If the\n"
            "variable in question makes use of the NUMS value this "
            "property\n"
            "will return the value, otherwise it will return None:\n"
            "\n"
            "   sum.smspec_node(\"FOPT\").num     => None\n"
            "   sum.smspec_node(\"BPR:1000\").num => 1000\n"
            "\n")

lib/resdata/smspec_node_pybind.cpp:116

Typos in the is_rate docstring: "conecpt" → "concept" and "variabel" → "variable".

        .def("is_rate", &rd::smspec_node::is_rate,
             "Will check if the variable in question is a rate variable.\n"
             "\n"
             "The conecpt of rate variabel is important (internally) when\n"
             "interpolation values to arbitrary times.\n")

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node_pybind.cpp:104

The get_num docstring examples imply FOPT.num is always None, but nodes created via Summary.writer().add_variable() can expose num == 0 (and need_nums() becomes true when num != SMSPEC_NUMS_INVALID). This makes the example misleading and also leaves a couple of typos in the is_rate docstring.

            "Returns the NUMS value for this keyword; or None.\n"
            "\n"
            "Many of the summary keywords have an integer stored in the\n"
            "vector NUMS as an attribute, i.e. the block properties have\n"
            "the global index of the cell in the nums vector. If the\n"

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node_pybind.cpp:110

The get_num() docstring claims FOPT returns None, but nodes created programmatically (e.g. via Summary.writer().add_variable("FOPT", ...)) can have need_nums()==true and return 0 (see new tests asserting node.num == 0). Please clarify the docstring to avoid giving users a false invariant.

            "Returns the NUMS value for this keyword; or None.\n"
            "\n"
            "Many of the summary keywords have an integer stored in the\n"
            "vector NUMS as an attribute, i.e. the block properties have\n"
            "the global index of the cell in the nums vector. If the\n"
            "variable in question makes use of the NUMS value this "
            "property\n"
            "will return the value, otherwise it will return None:\n"
            "\n"
            "   sum.smspec_node(\"FOPT\").num     => None\n"
            "   sum.smspec_node(\"BPR:1000\").num => 1000\n"

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

lib/resdata/smspec_node_pybind.cpp:107

The get_num()/num docstring currently implies the "FOPT" example always returns None, but nodes created via Summary.writer().add_variable("FOPT", ...) can have NUMS explicitly set (tests in this PR assert node.num == 0). Please clarify the sentence so it doesn’t overstate the guarantee.

            "will return the value, otherwise it will return None:\n"

lib/resdata/smspec_node_pybind.cpp:116

Docstring has spelling/grammar issues ("conecpt", "variabels", and the sentence fragment on the next line). This is user-facing help text, so it should be corrected.

             "The conecpt of rate variabels is important (internally) when\n"
             "interpolation values to arbitrary times.\n")

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node_pybind.cpp:116

Docstring has spelling/grammar issues: "conecpt" and "variabels" should be "concept" and "variables", and the sentence should read naturally (e.g. "interpolating values").

             "The conecpt of rate variabels is important (internally) when\n"
             "interpolation values to arbitrary times.\n")

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node_pybind.cpp:98

The get_num() docstring example says sum.smspec_node("FOPT").num => None, but this PR’s tests demonstrate valid nodes where FOPT has NUMS (e.g. writer-created variables) and .num returns 0. Update the wording/example to reflect that the property can return an integer (including 0) when need_nums() is true.

            "will return the value, otherwise it will return None:\n"
            "\n"
            "   sum.smspec_node(\"FOPT\").num     => None\n"
            "   sum.smspec_node(\"BPR:1000\").num => 1000\n"

MagnusSletten

Nice work! A couple of mostly minor comments. The hashing functionality is the most important to double check.

The operators would throw a TypeError before this fix, but now they follow the usual conventions of returning NotImplemented.

Also removes the ones that were unused and moves the ones only used in smspec_node.cpp there.

Several paths in smspec_noe.cmp would assume that values were not nullptr. This fixes this issue by instead comparing the private std::strings directly.

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/resdata/smspec_node_pybind.cpp:111

Spelling/grammar in the is_rate docstring: "conecpt" / "variabels" / "interpolation values".

             "The conecpt of rate variabels is important (internally) when\n"
             "interpolation values to arbitrary times.\n")

eivindjahren changed the base branch from main to pybind_fault July 23, 2026 13:02

eivindjahren force-pushed the fix_smspec_node branch from 746afed to 943e29b Compare July 23, 2026 13:04

eivindjahren requested a review from Copilot July 23, 2026 13:04

Copilot started reviewing on behalf of eivindjahren July 23, 2026 13:04 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

eivindjahren requested a review from Copilot July 23, 2026 13:23

Copilot started reviewing on behalf of eivindjahren July 23, 2026 13:24 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread lib/include/resdata/smspec_node.hpp Outdated

eivindjahren force-pushed the fix_smspec_node branch from 943e29b to 345e19a Compare July 23, 2026 13:53

eivindjahren requested a review from Copilot July 23, 2026 13:54

Copilot started reviewing on behalf of eivindjahren July 23, 2026 13:54 View session

eivindjahren force-pushed the fix_smspec_node branch from 345e19a to e998840 Compare July 23, 2026 13:55

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread lib/include/resdata/smspec_node.hpp

eivindjahren force-pushed the pybind_fault branch from c360269 to 5bbc240 Compare July 23, 2026 15:20

Base automatically changed from pybind_fault to main July 23, 2026 15:26

eivindjahren added 3 commits July 23, 2026 17:27

Remove usage of old smspec_node api

1bce43e

Rename smspec_node test

b8d56ef

Fix a bug where the IS_DUMMY_WELL was called before nullptr check

a739d04

eivindjahren force-pushed the fix_smspec_node branch from 0c87b6e to fee2a36 Compare July 23, 2026 15:27

eivindjahren requested a review from Copilot July 23, 2026 15:53

Copilot started reviewing on behalf of eivindjahren July 23, 2026 15:53 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread python/resdata/summary/rd_smspec_node.pyi Outdated

Comment thread python/resdata/summary/rd_smspec_node.pyi Outdated

eivindjahren force-pushed the fix_smspec_node branch from bb44701 to c0828ce Compare July 23, 2026 16:44

eivindjahren requested a review from Copilot July 23, 2026 16:44

Copilot started reviewing on behalf of eivindjahren July 23, 2026 16:44 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread python/resdata/summary/rd_smspec_node.pyi Outdated

eivindjahren force-pushed the fix_smspec_node branch from c0828ce to 3af14d7 Compare July 23, 2026 17:23

eivindjahren requested a review from Copilot July 23, 2026 17:23

Copilot started reviewing on behalf of eivindjahren July 23, 2026 17:24 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

eivindjahren force-pushed the fix_smspec_node branch from 3af14d7 to 02f1973 Compare July 23, 2026 19:44

eivindjahren requested a review from Copilot July 23, 2026 19:46

Copilot started reviewing on behalf of eivindjahren July 23, 2026 19:46 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread lib/resdata/smspec_node.cpp

MagnusSletten reviewed Jul 24, 2026

View reviewed changes

Comment thread lib/resdata/smspec_node_pybind.cpp

Comment thread lib/resdata/rd_sum_pybind.cpp

Comment thread python/resdata/summary/rd_smspec_node.py

eivindjahren added 9 commits July 24, 2026 08:40

Use pybind to define smspec_node

367a8d0

Remove extern "C" from smspec_node.hpp and remove old API

d9e39da

Fix smspec_node operators

fa8e916

The operators would throw a TypeError before this fix, but now they follow the usual conventions of returning NotImplemented.

Fix spelling in documentation

815d8fb

Add type-stubs for rd_smspec_node

efd96c1

Convert c macros in smspec_node to constexpr

e53afa1

Also removes the ones that were unused and moves the ones only used in smspec_node.cpp there.

Use inline definitions for simple getters

4d2d40b

Remove unused smspec_node methods

2e3f72f

Initial property based test

27b01e2

eivindjahren force-pushed the fix_smspec_node branch 2 times, most recently from 1420dbd to 9ab3684 Compare July 24, 2026 07:44

Fix nullptr dereference in smspec_node comparison

88f7f4c

Several paths in smspec_noe.cmp would assume that values were not nullptr. This fixes this issue by instead comparing the private std::strings directly.

eivindjahren force-pushed the fix_smspec_node branch from 9ab3684 to 88f7f4c Compare July 24, 2026 07:46

eivindjahren requested a review from Copilot July 24, 2026 07:46

Copilot started reviewing on behalf of eivindjahren July 24, 2026 07:47 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread lib/resdata/smspec_node_pybind.cpp

eivindjahren merged commit 14c576f into main Jul 24, 2026
12 checks passed

eivindjahren deleted the fix_smspec_node branch July 24, 2026 08:12

Uh oh!

Conversation

eivindjahren commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

eivindjahren Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

MagnusSletten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eivindjahren commented Jul 23, 2026 •

edited

Loading

eivindjahren Jul 23, 2026 •

edited

Loading