Skip to content

Fixes to assertions and overrides#106

Merged
pelesh merged 7 commits into
developfrom
nicholson/assert-override-fixes
May 14, 2025
Merged

Fixes to assertions and overrides#106
pelesh merged 7 commits into
developfrom
nicholson/assert-override-fixes

Conversation

@nkoukpaizan
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
WiktoriaZielinskaORNL pushed a commit that referenced this pull request Jul 23, 2025
* size_quad() --> sizeQuadrature().

* Add missing overrides in IdaTests.

* Fix time assert with tolerance within a namespace.

* Use GridKit's isEqual in example2 time stamp assertion.

---------

Co-authored-by: nkoukpaizan <nkoukpaizan@users.noreply.github.com>
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