Better handle __repr__ (#39) - #42
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several representations reference the nonexistent atlas4py.mesh module, and most new outputs remain untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves atlas4py object representations for clearer debugging and interactive use.
Changes:
- Standardizes representations across core bindings.
- Makes point representations round-trip safe.
- Adds parameterized point representation tests.
File summaries
| File | Description |
|---|---|
src/atlas4py/_atlas4py.cpp |
Adds and updates binding representations. |
tests/test_bindings.py |
Tests point representation round trips. |
Review details
Suppressed comments (7)
src/atlas4py/_atlas4py.cpp:387
StructuredMeshGeneratoris exported from the root package, and there is noatlas4py.meshsubmodule. Use its actual public path so the new representation does not advertise a nonexistent symbol.
return "<atlas4py.mesh.StructuredMeshGenerator>";
src/atlas4py/_atlas4py.cpp:414
- Both branches identify this root-level binding as
atlas4py.mesh.IrregularConnectivity, butmeshis not a Python submodule. The representation should use the class's exported path,atlas4py.IrregularConnectivity.
.def("__repr__", []( mesh::IrregularConnectivity const& c ) {
src/atlas4py/_atlas4py.cpp:433
- Both branches use the nonexistent path
atlas4py.mesh.BlockConnectivity; this class is registered on the root module and exposed asatlas4py.BlockConnectivity.
.def("__repr__", []( mesh::BlockConnectivity const& c ) {
src/atlas4py/_atlas4py.cpp:461
- The representation points to
atlas4py.mesh.MultiBlockConnectivity, although nomeshPython submodule exists and this binding is exported atatlas4py.MultiBlockConnectivity.
.def("__repr__", []( mesh::MultiBlockConnectivity const& c ) {
src/atlas4py/_atlas4py.cpp:481
Nodesis a root-level Python binding, not a member of anatlas4py.meshsubmodule. This should showatlas4py.Nodesto avoid reporting a nonexistent public path.
oss << "<atlas4py.mesh.Nodes size=" << n.size() << ">";
src/atlas4py/_atlas4py.cpp:496
- This class is exported as
atlas4py.HybridElements;atlas4py.mesh.HybridElementsis not available because nomeshsubmodule is created.
oss << "<atlas4py.mesh.HybridElements size=" << he.size() << ">";
src/atlas4py/_atlas4py.cpp:600
Topologyis registered directly on the root module, soatlas4py.mesh.Nodes.Topologyis not a valid Python path. Report its actual exported name,atlas4py.Topology.
return "<atlas4py.mesh.Nodes.Topology>";
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Point representations fail round trips for non-finite coordinates, and the new stream usage lacks its required header.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/atlas4py/_atlas4py.cpp:234
- This is not round-trip safe for all accepted
doublevalues: Python renders non-finite values as bareinf,-inf, ornan, so evaluating the resulting point representation raisesNameError. Format non-finite coordinates as evaluable expressions (for example,float('inf')) and cover them in the round-trip test.
This issue also appears on line 241 of the same file.
src/atlas4py/_atlas4py.cpp:241
PointXYhas the same round-trip failure for accepted non-finite coordinates:nb::str(nb::float_(...))emits bareinf/nan, whichevalcannot resolve. Use an evaluable representation for special floating-point values and add corresponding test cases.
return "atlas4py.PointXY(x="_s + nb::str( nb::float_( p.x() ) ) + ", y="_s + nb::str( nb::float_( p.y() ) ) + ")"_s;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
This pull request tries to address #39 , by standardizing and improving the
__repr__string representations for many core classes, making debugging and interactive usage more informative and user-friendly. It also adds a new test to ensure that the__repr__output for point classes is round-trip safe (i.e., objects can be reconstructed from their string representation).Key changes include:
Improved and Standardized
__repr__Methods:__repr__implementations for numerous classes (e.g.,Field,Mesh,StructuredMeshGenerator,IrregularConnectivity,BlockConnectivity,MultiBlockConnectivity,Nodes,HybridElements,FunctionSpace,EdgeColumns,NodeColumns,CellColumns,Topology, andGmsh). These representations now follow a consistent, Pythonic format (e.g.,<atlas4py.ClassName ...>), often including key attributes for easier inspection.Testing Improvements:
__repr__methods return the expected strings for all major classes, including round-trip tests for point types and coverage for mesh/connectivity/function space objects.Code Quality and Consistency: