Skip to content

Fixes to assertions and overrides #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 14, 2025
Merged

Conversation

nkoukpaizan
Copy link
Collaborator

@nkoukpaizan nkoukpaizan commented May 13, 2025

Description

Building in debug mode revealed a few asserts that were failing

One failing assertion was due to a member function name change:

GridKit/src/Solver/Optimization/DynamicConstraint.cpp:31:22: error: no member named 'size_quad' in 'GridKit::Model::Evaluator<double, long>'
   31 |       assert(model_->size_quad() == 1);
      |              ~~~~~~  ^

Another failing assertion was due to a comparison between two floating point numbers:

Assertion failed: (t == other.t), function operator-=, file example2.cpp, line 42.

Additionally, LLVM tends to issue more warnings, such as overrides

GridKit/tests/UnitTests/Solver/Dynamic/IdaTests.hpp:113:29: warning: 'y' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  113 |       std::vector<ScalarT>& y()
      |

Proposed changes

  • size_quad() --> sizeQuadrature()
  • Use a tolerance to compare the floating point values in example2. Also put the data within a namespace.
  • Adding missing overrides

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

Further comments

I suggest ignoring whitespaces for the diff for the example*hpp files.

@nkoukpaizan nkoukpaizan self-assigned this May 13, 2025
@nkoukpaizan nkoukpaizan added the bug Something isn't working label May 13, 2025
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass when built with GCC and Clang. Good to merge.

Comment on lines 42 to 43
assert((t - other.t) < Example2::reference_tol);
gen2speed -= other.gen2speed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using GridKit's comparison for floating point numbers:

#include <Utilities/Testing.hpp>

...

assert(isEqual(t, other.t));

@pelesh pelesh merged commit c214f9c into develop May 14, 2025
3 checks passed
@pelesh pelesh deleted the nicholson/assert-override-fixes branch June 10, 2025 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants