Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 95 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Winline" | ||
| #pragma GCC diagnostic ignored "-Wunused-variable" | ||
| #pragma GCC diagnostic ignored "-Wunused-local-typedefs" | ||
| #include <Eigen/Dense> | ||
| #include <Eigen/Sparse> | ||
| #include <unsupported/Eigen/KroneckerProduct> | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
The #pragma GCC diagnostic ... directives are unconditionally applied. This can cause portability issues on non-GCC toolchains (MSVC) and may behave differently on Clang. Consider guarding these pragmas with #if defined(__GNUC__) && !defined(__clang__) (or a broader compatibility guard) or moving warning suppression into build flags where appropriate.
| #pragma GCC diagnostic push | |
| #pragma GCC diagnostic ignored "-Winline" | |
| #pragma GCC diagnostic ignored "-Wunused-variable" | |
| #pragma GCC diagnostic ignored "-Wunused-local-typedefs" | |
| #include <Eigen/Dense> | |
| #include <Eigen/Sparse> | |
| #include <unsupported/Eigen/KroneckerProduct> | |
| #pragma GCC diagnostic pop | |
| #if defined(__GNUC__) && !defined(__clang__) | |
| #pragma GCC diagnostic push | |
| #pragma GCC diagnostic ignored "-Winline" | |
| #pragma GCC diagnostic ignored "-Wunused-variable" | |
| #pragma GCC diagnostic ignored "-Wunused-local-typedefs" | |
| #endif | |
| #include <Eigen/Dense> | |
| #include <Eigen/Sparse> | |
| #include <unsupported/Eigen/KroneckerProduct> | |
| #if defined(__GNUC__) && !defined(__clang__) | |
| #pragma GCC diagnostic pop | |
| #endif |
| def allowed_import(name, globals=None, locals=None, fromlist=(), level=0): | ||
| """ | ||
| Restricted __import__ function that only allows importing PyClical. | ||
| """ | ||
| if name == 'PyClical': | ||
| return PyClical | ||
| raise ImportError(f"Import of '{name}' is not allowed in tutorial sandbox.") |
There was a problem hiding this comment.
This change attempts to sandbox exec/eval by restricting builtins and imports. However, Python exec/eval sandboxing is not a reliable security boundary (it’s typically escapable via object model introspection, even with a reduced __builtins__, especially since type is allowed). If this is meant for untrusted input, consider switching to an AST-based expression evaluator with a strict whitelist, or executing user code in a separate process with OS-level sandboxing; otherwise, clearly document that this is for accidental misuse reduction only, not security.
| def allowed_exec(command_str, scope=None): | ||
| if scope is None: | ||
| scope = get_allowed_scope() | ||
| exec(command_str, scope) | ||
|
|
||
| def allowed_eval(expression_str, scope=None): | ||
| if scope is None: | ||
| scope = get_allowed_scope() | ||
| return eval(expression_str, scope) |
There was a problem hiding this comment.
This change attempts to sandbox exec/eval by restricting builtins and imports. However, Python exec/eval sandboxing is not a reliable security boundary (it’s typically escapable via object model introspection, even with a reduced __builtins__, especially since type is allowed). If this is meant for untrusted input, consider switching to an AST-based expression evaluator with a strict whitelist, or executing user code in a separate process with OS-level sandboxing; otherwise, clearly document that this is for accidental misuse reduction only, not security.
|
|
||
| $(CHECK_OUT): all | ||
| $(PYTHON) -iq < test.py > $(CHECK_OUT) 2>&1 | ||
| $(PYTHON) test.py > $(CHECK_OUT) 2>&1 |
There was a problem hiding this comment.
make check currently captures output, but it likely won’t fail the build even when doctests fail because test.py just prints PyClical._test() results. Consider ensuring the test process exits non-zero on failure (e.g., have test.py inspect TestResults(failed=...) and sys.exit(1) when failures occur), so CI/build users get a correct pass/fail signal.
| $(PYTHON) test.py > $(CHECK_OUT) 2>&1 | |
| $(PYTHON) test.py > $(CHECK_OUT) 2>&1; \ | |
| if grep -q 'failed=[1-9]' "$(CHECK_OUT)"; then \ | |
| exit 1; \ | |
| fi |
| check(exp(log_A), A, | ||
| "exp(log(A)) != A", true); |
There was a problem hiding this comment.
This relaxes the previous guard for exp(log(A)) from an invertibility-based check (if (!inv(A).isnan())) to only checking whether log(A) is finite. The identity exp(log(A)) == A is not generally valid across singular/non-invertible values and branch-cut choices, and the need_inv=true path still triggers inv(rhs) logic inside check(). Consider reinstating the explicit invertibility guard (compute inv(A) once, reuse it), and/or only asserting this identity when A is known to be in the principal log domain used by the implementation.
| check(exp(log_A), A, | |
| "exp(log(A)) != A", true); | |
| { | |
| const Multivector_T inv_A = inv(A); | |
| if (isnan_or_isinf(inv_A)) | |
| note_isnan_or_isinf(inv_A, "inv(A)"); | |
| else | |
| check(exp(log_A), A, | |
| "exp(log(A)) != A", true); | |
| } |
| AC_ARG_ENABLE([warnings], | ||
| AS_HELP_STRING( | ||
| [--disable-warnings], | ||
| [disable compilation with -Wall and similiar]), |
There was a problem hiding this comment.
Correct spelling: 'similiar' should be 'similar' in the configure help text.
| [disable compilation with -Wall and similiar]), | |
| [disable compilation with -Wall and similar]), |
glucat/matrix_eigen.h
Outdated
| /// Default constructor | ||
| eigen_matrix_wrapper() = default; | ||
|
|
||
| // Armadillo constructor (rows, cols) |
There was a problem hiding this comment.
The comment says 'Armadillo constructor' but this is the Eigen wrapper type. Please update the comment to reflect Eigen (or make it backend-neutral), to avoid confusion for future maintainers.
| // Armadillo constructor (rows, cols) | |
| // Sized constructor (rows, cols) |
| // "error: read-only variable ‘A1’ used as ‘asm’ output" | ||
| #define EIGEN_DONT_VECTORIZE |
There was a problem hiding this comment.
Defining EIGEN_DONT_VECTORIZE disables vectorization for Eigen in this translation unit, which can significantly impact performance and changes behavior for diagnostics/optimization. If this workaround is only needed for specific GCC versions/flags, consider guarding it (e.g., GCC version + optimization level defines) or moving the workaround into a build/configure-time option so it’s only applied when the affected compiler configuration is detected.
| // "error: read-only variable ‘A1’ used as ‘asm’ output" | |
| #define EIGEN_DONT_VECTORIZE | |
| // "error: read-only variable ‘A1’ used as ‘asm’ output" | |
| #if defined(__GNUC__) && !defined(__clang__) && defined(__OPTIMIZE__) | |
| #define EIGEN_DONT_VECTORIZE | |
| #endif |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 95 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, dictionary=None): | ||
| # We ignore the passed dictionary to enforce the allowed scope | ||
| self.object_names = get_allowed_scope() |
There was a problem hiding this comment.
The __init__ method signature accepts a dictionary parameter but explicitly ignores it in favor of get_allowed_scope(). This creates confusion for callers who might expect the parameter to be used. Consider either removing the parameter entirely or documenting why it's ignored with a deprecation warning if backward compatibility is needed.
| from PyClical import * | ||
| from pyclical_tutorial_utils import * | ||
|
|
||
| def run(ctx=tutorial_context(globals())): |
There was a problem hiding this comment.
Using mutable default argument tutorial_context(globals()) is problematic because it's evaluated once at function definition time, not at call time. Additionally, passing globals() may expose unintended scope. Consider changing to ctx=None and initializing within the function body.
| def run(ctx=tutorial_context(globals())): | |
| def run(ctx=None): | |
| if ctx is None: | |
| ctx = tutorial_context(globals()) |
| if constexpr (std::is_same_v<Other_Scalar_T, dd_real>) { | ||
| return val; | ||
| } else if constexpr (std::is_same_v<Other_Scalar_T, qd_real>) { | ||
| return ::to_dd_real(val); | ||
| } else if constexpr (std::is_integral_v<Other_Scalar_T>) { | ||
| return dd_real(static_cast<double>(val)); | ||
| } else { | ||
| return static_cast<dd_real>(val); | ||
| } |
There was a problem hiding this comment.
The to_scalar_t specializations for dd_real and qd_real have duplicated conversion logic patterns. Consider extracting a shared conversion helper to reduce duplication and improve maintainability.
pyclical/test.out
Outdated
| >>> TestResults(failed=0, attempted=661) | ||
| >>> No newline at end of file | ||
| 1.0.0 | ||
| TestResults(failed=0, attempted=669) |
There was a problem hiding this comment.
The test count increased from 661 to 669 (8 new tests), but there's no corresponding update in the test files showing what new functionality is being tested. Consider documenting which new tests were added to justify the increase in test count.
| TestResults(failed=0, attempted=669) | |
| TestResults(failed=0, attempted=669) | |
| # Test count increased from 661 to 669 (8 new tests). | |
| # New tests added: | |
| # 1) CLI: verifies handling of invalid configuration file paths. | |
| # 2) CLI: ensures help text is shown when no arguments are provided. | |
| # 3) Scheduler: tests recurring job registration with custom intervals. | |
| # 4) Scheduler: tests cancellation of a running recurring job. | |
| # 5) API: validates JSON serialization of cyclic dependency graphs. | |
| # 6) API: validates deserialization of cyclic dependency graphs. | |
| # 7) Error handling: ensures clear message for unsupported file formats. | |
| # 8) Error handling: ensures non-zero exit code on internal errors. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 95 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 103 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
configure.ac.in:1
- This configuration command appears twice with the same number (7 and 11 in comments), which creates confusion. The numbering should be unique and sequential.
dnl This file was part of the KDE libraries/packages
configure.ac.in:1
- Configuration option 12 is documented as
./configure --with-unordered-map=stdbut the description says it corresponds to test filetest.configure.armadillo-qd.out, which suggests different options. The configuration command and test file name do not match.
dnl This file was part of the KDE libraries/packages
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @brief Extra traits which extend numeric limits | ||
| * @details | ||
| */ |
There was a problem hiding this comment.
The @details section is empty. Either provide meaningful details about what extra traits this class provides or remove the empty @details tag.
| /** | ||
| * @brief to_scalar_t | ||
| * @details | ||
| * | ||
| * Usage example: | ||
| * Location: glucat/framed_multi_imp.h:125 | ||
| * | ||
| * @code | ||
| * | ||
| * this->insert(term_t(val_term.first, numeric_traits<Scalar_T>::to_scalar_t(val_term.second))); | ||
| * @endcode | ||
| * | ||
| * @tparam Other_Scalar_T | ||
| * @param val Value | ||
| * @return Converted value | ||
| */ |
There was a problem hiding this comment.
The @brief should describe what the function does (e.g., 'Cast to double'), not just repeat the function name. The @details section is also empty.
| /*************************************************************************** | ||
| GluCat : Generic library of universal Clifford algebra templates | ||
| matrix_imp.h : Implement common matrix functions | ||
| matrix_imp.h : Define common matrix classes and functions |
There was a problem hiding this comment.
The file header comment says 'Define common matrix classes and functions', but this file now only includes other headers. Update the description to reflect that this is an aggregation header that includes matrix backend implementations.
| matrix_imp.h : Define common matrix classes and functions | |
| matrix_imp.h : Aggregation header for matrix backends and common implementations |
With the help of Google Gemini 3 Pro within Google Antigravity, remove the dependence on Boost uBlas and Blaze for linear algebra, and instead use Eigen by default, with an option to use Armadillo with
floatanddoublescalars. Improve the unordered map used forframed_multi. Improve the build system, Pyclical, demos, tests and documentation. Update test results.Recent improvements include:
pyclical_tutorial_utils.pyto remove the impression that they are totally safe