Conversation
- CMakeLists.txt: require aGrUM >= 3, find OpenMP - Drop unused NamedDAG.hxx include from CorrectedMutualInformation.hxx - Clarify NamedJunctionTree's variable-id vs clique-id name mapping - Refactor NamedDAG.cxx and TabuList.cxx - Update expected test outputs accordingly - Add detailed code audit (audit.md)
- JunctionTreeBernsteinCopula::setBinNumber() now recomputes cached state - NamedJunctionTree: replace assert() with an exception (release-safe) - Fix stale ResourceMap key name in factory docstring - Fix copy-pasted SWIG file header comment - Document the SWIG warning-suppression pragma
…pointers - Share namesFromData/idFromName/learnDAG/GetK via Utils across ContinuousPC, ContinuousMIIC and TabuList - Extract recordLatentCoupleIfNeeded() and legalChangesForPair() to remove duplicated blocks in ContinuousMIIC/TabuList - Reject empty samples in ContinuousTTest/CorrectedMutualInformation instead of silently propagating NaN - Reject TabuList(restarts=0) instead of underflowing - Wrap rank_'s shared_ptr<tuple<...>> in a RankedTriple class with named accessors (gum::Heap requires copyable elements, so unique_ptr is not an option) - Add license/OTAGRUM_API to Greater.hxx, document why DeriveDAG is hidden from SWIG
- Add C++ tests for Greater and JunctionTreeBernsteinCopulaFactory - Add Python tests for ContinuousTTest, CorrectedMutualInformation, NamedDAG and JunctionTreeBernsteinCopulaFactory - Reactivate disabled Utils.py tests and ContinuousBayesianNetwork's genereData() - Add empty-sample/invalid-input exception checks to ContinuousPC, ContinuousMIIC and TabuList Python tests
Targets: cpp, py, cpptest, pytest, all, clean.
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe change updates the project to version 0.15, adds standardized CMake build targets, centralizes graph and estimator utilities, revises learning algorithms and validation, and expands C++ and Python test coverage. ChangesRelease and build updates
Core library changes
Validation and interfaces
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Learner
participant Utils
participant AGrUM
participant NamedDAG
Learner->>Utils: resolve names and derive DAG
Utils->>AGrUM: propagate PDAG with MeekRules
AGrUM-->>Utils: derived DAG
Utils->>NamedDAG: construct named graph
NamedDAG-->>Learner: return learned DAG
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a standard
testtarget.To conform with standard Makefile conventions and address the static analysis warning, consider adding a
testtarget that runs bothcpptestandpytest.♻️ Proposed refactor
-.PHONY: all cpp py cpptest pytest clean +.PHONY: all cpp py cpptest pytest test clean all: cpp py + +test: cpptest pytest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 3 - 5, Add a standard test target to the Makefile that depends on and runs both existing cpptest and pytest targets, while preserving their current behavior and declarations.Source: Linters/SAST tools
python/test/t_ContinuousMIIC_std.py (1)
104-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid catching blind exceptions in validation tests.
Across several test scripts,testEmptySampleRaisescatches the baseExceptionclass. This can inadvertently swallow unrelated errors—such as aNameErrororAttributeErrorresulting from a typo inside thetryblock—causing the test to falsely pass even if the constructor wasn't called correctly. Consider catching the specific exception raised by OpenTURNS/SWIG.
python/test/t_ContinuousMIIC_std.py#L104-L110: Replaceexcept Exception:withexcept (ValueError, RuntimeError):or the specific expected exception.python/test/t_ContinuousPC_std.py#L49-L55: Replaceexcept Exception:withexcept (ValueError, RuntimeError):or the specific expected exception.python/test/t_ContinuousTTest_std.py#L39-L45: Replaceexcept Exception:withexcept (ValueError, RuntimeError):or the specific expected exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/test/t_ContinuousMIIC_std.py` around lines 104 - 110, Replace the broad exception handling in testEmptySampleRaises with ValueError, RuntimeError, or the specific expected OpenTURNS/SWIG exception in python/test/t_ContinuousMIIC_std.py:104-110, python/test/t_ContinuousPC_std.py:49-55, and python/test/t_ContinuousTTest_std.py:39-45, so unrelated errors are not swallowed.Source: Linters/SAST tools
lib/src/ContinuousMIIC.cxx (1)
341-353: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRoute the latent-couple appends through the helper
The four orientation-propagation branches still append directly instead of usingrecordLatentCoupleIfNeeded(...); centralizing that bookkeeping keeps the handling consistent with the other paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/ContinuousMIIC.cxx` around lines 341 - 353, The four orientation-propagation branches should stop appending latent couples directly and call ContinuousMIIC::recordLatentCoupleIfNeeded(...) instead. Replace each direct latent_couples_ append with the helper, passing the corresponding node pair, while preserving each branch’s existing orientation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/src/ContinuousBayesianNetworkFactory_doc.i`:
- Around line 42-43: Fix the spacing in the documentation text for
ContinuousBayesianNetworkFactory-UseBetaCopula by adding a space before the
:class:`openturns.BernsteinCopulaFactory` Sphinx directive, preserving the
existing wording and formatting otherwise.
---
Nitpick comments:
In `@lib/src/ContinuousMIIC.cxx`:
- Around line 341-353: The four orientation-propagation branches should stop
appending latent couples directly and call
ContinuousMIIC::recordLatentCoupleIfNeeded(...) instead. Replace each direct
latent_couples_ append with the helper, passing the corresponding node pair,
while preserving each branch’s existing orientation logic.
In `@Makefile`:
- Around line 3-5: Add a standard test target to the Makefile that depends on
and runs both existing cpptest and pytest targets, while preserving their
current behavior and declarations.
In `@python/test/t_ContinuousMIIC_std.py`:
- Around line 104-110: Replace the broad exception handling in
testEmptySampleRaises with ValueError, RuntimeError, or the specific expected
OpenTURNS/SWIG exception in python/test/t_ContinuousMIIC_std.py:104-110,
python/test/t_ContinuousPC_std.py:49-55, and
python/test/t_ContinuousTTest_std.py:39-45, so unrelated errors are not
swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77cf8568-4022-42e0-af01-4ca16c0e0cfb
📒 Files selected for processing (54)
AUTHORSCMakeLists.txtMakefileVERSIONdistro/debian/changelogdistro/rpm/otagrum.speclib/src/ContinuousMIIC.cxxlib/src/ContinuousPC.cxxlib/src/ContinuousTTest.cxxlib/src/CorrectedMutualInformation.cxxlib/src/Greater.cxxlib/src/JunctionTreeBernsteinCopula.cxxlib/src/NamedDAG.cxxlib/src/NamedJunctionTree.cxxlib/src/TabuList.cxxlib/src/Utils.cxxlib/src/otagrum/ContinuousMIIC.hxxlib/src/otagrum/CorrectedMutualInformation.hxxlib/src/otagrum/Greater.hxxlib/src/otagrum/NamedDAG.hxxlib/src/otagrum/NamedJunctionTree.hxxlib/src/otagrum/TabuList.hxxlib/src/otagrum/Utils.hxxlib/test/CMakeLists.txtlib/test/t_ContinuousBayesianNetwork_std.cxxlib/test/t_ContinuousBayesianNetwork_std.expoutlib/test/t_ContinuousPC_std.expoutlib/test/t_Greater_std.cxxlib/test/t_Greater_std.expoutlib/test/t_JunctionTreeBernsteinCopulaFactory_std.cxxlib/test/t_JunctionTreeBernsteinCopulaFactory_std.expoutlib/test/t_Utils_std.expoutpython/src/ContinuousBayesianNetworkFactory_doc.ipython/src/NamedDAG.ipython/src/NamedDAG_doc.ipython/src/__init__.pypython/src/otagrum_agrum.ipython/test/CMakeLists.txtpython/test/t_ContinuousMIIC_std.expoutpython/test/t_ContinuousMIIC_std.pypython/test/t_ContinuousPC_std.expoutpython/test/t_ContinuousPC_std.pypython/test/t_ContinuousTTest_std.expoutpython/test/t_ContinuousTTest_std.pypython/test/t_CorrectedMutualInformation_std.expoutpython/test/t_CorrectedMutualInformation_std.pypython/test/t_JunctionTreeBernsteinCopulaFactory_std.expoutpython/test/t_JunctionTreeBernsteinCopulaFactory_std.pypython/test/t_NamedDAG_std.expoutpython/test/t_NamedDAG_std.pypython/test/t_TabuList_std.expoutpython/test/t_TabuList_std.pypython/test/t_Utils_std.expoutpython/test/t_Utils_std.py
💤 Files with no reviewable changes (1)
- lib/src/otagrum/CorrectedMutualInformation.hxx
| - *'ContinuousBayesianNetworkFactory-UseBetaCopula'* to indicate if the | ||
| estimated copula should be a Beta copula if a:class:`openturns.BernsteinCopulaFactory` is provided. Default value is *True*. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix spacing for Sphinx directive.
The :class: directive on line 43 is missing a space before it, which will prevent Sphinx from rendering the cross-reference correctly.
📝 Proposed fix
- estimated copula should be a Beta copula if a:class:`openturns.BernsteinCopulaFactory` is provided. Default value is *True*.
+ estimated copula should be a Beta copula if a :class:`~openturns.BernsteinCopulaFactory` is provided. Default value is *True*.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - *'ContinuousBayesianNetworkFactory-UseBetaCopula'* to indicate if the | |
| estimated copula should be a Beta copula if a:class:`openturns.BernsteinCopulaFactory` is provided. Default value is *True*. | |
| - *'ContinuousBayesianNetworkFactory-UseBetaCopula'* to indicate if the | |
| estimated copula should be a Beta copula if a :class:`~openturns.BernsteinCopulaFactory` is provided. Default value is *True*. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/src/ContinuousBayesianNetworkFactory_doc.i` around lines 42 - 43, Fix
the spacing in the documentation text for
ContinuousBayesianNetworkFactory-UseBetaCopula by adding a space before the
:class:`openturns.BernsteinCopulaFactory` Sphinx directive, preserving the
existing wording and formatting otherwise.
agrum's BIFReader.h/BIFWriter.h pull in <windows.h> on mingw, which #defines GetClassName to GetClassNameA/W. In unity builds this leaks into later translation units (e.g. NamedDAG.cxx), breaking OT's CLASSNAMEINIT macro whose declaration/definition then mismatch.
| cd $(BUILD_DIR) && ctest --output-on-failure -R '^pyinstallcheck_' | ||
|
|
||
| clean: | ||
| rm -rf $(BUILD_DIR) |
There was a problem hiding this comment.
imho this is redundant with CMakePresets file and should be dropped
| message (STATUS "Found OpenTURNS: ${OpenTURNS_DIR} (found version \"${OpenTURNS_VERSION}\")") | ||
|
|
||
| find_package (aGrUM 2 CONFIG REQUIRED) | ||
| find_package (OpenMP) |
There was a problem hiding this comment.
looks like an openmp public dependency should be added in agrum cmake config with find_dependencies instead
| otagrum.ContinuousMIIC(ot.Sample()) | ||
| print("empty sample raises : fail (no exception)") | ||
| except Exception: | ||
| print("empty sample raises : OK") |
There was a problem hiding this comment.
use openturns.testing.assert_raises instead
| otagrum.ContinuousTTest(ot.Sample()) | ||
| print("empty sample raises : fail (no exception)") | ||
| except Exception: | ||
| print("empty sample raises : OK") |
| try: | ||
| otagrum.CorrectedMutualInformation(ot.Sample()) | ||
| print("empty sample raises : fail (no exception)") | ||
| except Exception: |
| try: | ||
| otagrum.NamedDAG(bn) | ||
| print("non contiguous ids raises : fail (no exception)") | ||
| except Exception: |
| try: | ||
| otagrum.TabuList(data, restarts=0) | ||
| print("restarts=0 raises : fail (no exception)") | ||
| except Exception: |
|
included in #106 |
2 participants
Summary by CodeRabbit
NamedDAGname-to-ID lookup support.