Fixes #1863 remove check for monotonically increasing time values?#2764
Conversation
📝 WalkthroughWalkthroughRemoved strict monotonicity checks and related APIs in BaseGrid, changed index-lookup semantics and averaging in DataColumn, deleted TimeNotStrictlyMonotoneException and its wrapping, refactored chart tooltip logic to ChartHitInfo, removed repository insert/swap APIs, and updated/removed tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@src/OSPSuite.Core/Domain/Data/DataColumn.cs`:
- Around line 243-258: The averageFor method compares floats with exact equality
(baseGridAtIndex vs x) which can fail due to floating-point precision; modify
averageFor (and iterations over BaseGrid.Values) to use a tolerance-based
comparison (e.g., Math.Abs(BaseGrid[i] - baseGridAtIndex) <= epsilon) or iterate
by index using BaseGrid.IndexOf/Count to find matching positions, accumulate
Values[i] for matches, and choose a small sensible epsilon (e.g., 1e-6f) defined
centrally; update any usages of BaseGrid.Values.Each in averageFor to use this
tolerant comparison to avoid missed matches.
In `@tests/OSPSuite.Core.Tests/Domain/DataColumnSpecs.cs`:
- Line 147: In the test assignment to relatedColumn.Values in DataColumnSpecs
(the array with values 0.1F, 0.2F, 0.1F, 02F), fix the typo by changing the last
element from 02F to 0.2F so the array reads new[] { 0.1F, 0.2F, 0.1F, 0.2F };
locate the assignment via the relatedColumn.Values reference and update that
literal.
🧹 Nitpick comments (2)
src/OSPSuite.Core/Domain/Data/DataColumn.cs (1)
224-224: Clarify or remove the uncertain comment.The comment "We can throw an exception here if the values are not monotonic?" reads as a question rather than documentation. If non-monotonic values should be handled silently (which is the PR's intent), consider removing this comment or converting it to a clear statement of the design decision.
✏️ Suggested clarification
- // We can throw an exception here if the values are not monotonic? + // Non-monotonic values are supported - find neighbors by value comparison int leftIndex = BaseGrid.IndexOfNextLowest(baseValue);tests/OSPSuite.Core.Tests/Domain/BaseGridSpecs.cs (1)
81-101: Test method names no longer match the API methods they test.The test methods
TestRightIndexOfandTestLeftIndexOfnow callIndexOfNextHighestandIndexOfNextLowestrespectively, but their names still reference the old method names. Consider renaming for clarity:
TestRightIndexOf→TestIndexOfNextHighestTestLeftIndexOf→TestIndexOfNextLowestAdditionally, since this PR removes the monotonicity requirement, consider adding test cases with non-monotonic grid values to verify the new lookup behavior works correctly when values are not sorted.
🔧 Suggested rename
[Observation] - public void TestRightIndexOf() + public void TestIndexOfNextHighest() { sut.Values = new[] { -2.0F, 0.5F, 1.0F, 1.5F }; Assert.AreEqual(1, sut.IndexOfNextHighest(0.5F)); Assert.AreEqual(2, sut.IndexOfNextHighest(0.6F)); Assert.AreEqual(4, sut.IndexOfNextHighest(1.6F)); Assert.AreEqual(0, sut.IndexOfNextHighest(-3F)); } [Observation] - public void TestLeftIndexOf() + public void TestIndexOfNextLowest() { sut.Values = new[] { -2.0F, 0.5F, 1.0F, 1.5F }; Assert.AreEqual(1, sut.IndexOfNextLowest(0.5F)); Assert.AreEqual(1, sut.IndexOfNextLowest(0.6F)); Assert.AreEqual(3, sut.IndexOfNextLowest(1.6F)); Assert.AreEqual(-1, sut.IndexOfNextLowest(-3F)); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/OSPSuite.Core/Domain/Data/DataColumn.cs`:
- Around line 224-226: Remove the unresolved question comment and replace it
with a short descriptive comment stating the expected behavior: that we no
longer enforce monotonicity and instead rely on
BaseGrid.IndexOfNextLowest(baseValue) and BaseGrid.IndexOfNextHighest(baseValue)
to return the nearest indices (or -1/null if none); update the comment near the
DataColumn method where leftIndex/rightIndex are computed to reflect this
decision (reference BaseGrid.IndexOfNextLowest and BaseGrid.IndexOfNextHighest
and the DataColumn class) so there is no ambiguity about error handling or
expectations.
| //check strong monotony of values first (values are sorted and unique) | ||
| for (int i = 1; i < value.Count; i++) | ||
| { | ||
| if (value[i] <= value[i - 1]) | ||
| { | ||
|
|
||
| var beforeValue =this.ConvertToDisplayUnit(value[i - 1]); | ||
| var afterValue = this.ConvertToDisplayUnit(value[i]); | ||
| throw new TimeNotStrictlyMonotoneException(beforeValue, afterValue, DisplayUnit.Name, Repository?.Name); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
should we at least check that the base grid values are sorted?
I think even in case of non-unique base grid we should sort the values before.
Otherwise, if when e.g. plotting observed data not only by dots but by dots+line - it could become ugly if the time values are not sorted. Also the IndexOfNextXXX routines below would be simpler.
Btw: use cases where not unique values (and not sorted values for sure) could make trouble are linear interpolations of data points, and those use cases must be checked once we allow non unique values:
- import of ph dependent solubility table
- import of formulation table
- Import data points of arbitrary table formula in MoBi
There was a problem hiding this comment.
yeah, sorted is a MUST.
Non unique values in table formula with activated "Use derivated values" will produce NaNs whatsoever pretty sure
There was a problem hiding this comment.
The more I think about it: non-unique values in all cases above must be forbidden.
It's only observed data (and maybe few other cases) where it could be allowed.
Any use case where table represents a function y=f(BaseGrid) should support only unique (and sorted) values.
There was a problem hiding this comment.
Shouldn't that then be controlled outside the base grid object? By the presenters or dtos of these particular usages? Or by another base grid type that does enforce.
There was a problem hiding this comment.
When plotting observed data we could make this ordering when creating the series points for the chart, only for observation repositories
It's better to sort once during the data import - then no need to sort every time later when the data is plotted, exported, whatsoever.
So in principle I would keep the code in BaseGrid.cs almost AS IS with the only exception that it does not check anymore that the values are unique (but still checks that they are sorted).
There was a problem hiding this comment.
I'm not sure how much we gain by enforcing it. We can have the importer sort the points to make the charts less cluttered if lines are enabled, but interpolating values that are not at basegrid points will still require averaging.
All the removed insert/insert range stuff is not used anywhere in Suite anyway.
There was a problem hiding this comment.
We can have the importer sort the points
yep, this should be done imo
There was a problem hiding this comment.
ok, what I mean is, I'll leave out the Exception throwing for non-increasing. Both uniqueness and increasing base grid are handled by the construction of the base grid and is not enforced by the base grid.
There was a problem hiding this comment.
ok, let's try it like this and see what happens :)
|
There are two tasks here:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/OSPSuite.Core.IntegrationTests/PKValuesCalculatorSpecs.cs`:
- Around line 99-101: The test currently uses .Single() on
_multipleDosingColumn.BaseGrid.IndexesOf(...) which will fail or be inconsistent
if duplicates exist; instead retrieve the indexes via
_multipleDosingColumn.BaseGrid.IndexesOf(_pkOptions.LastMinusOneInterval.EndValue.Value)
and compute the averaged value across those indexes like DataColumn.GetValue
does (i.e., if indexes.Any() calculate the mean of _multipleDosingColumn.Values
at those indexes) and assign that mean to lastValueBeforeLastDose rather than
using indexOfEndTimePointOfLastButOneInterval from .Single(); update references
to indexOfEndTimePointOfLastButOneInterval accordingly.
In `@tests/OSPSuite.Core.Tests/Domain/BaseGridSpecs.cs`:
- Around line 74-102: Replace NUnit assertions in TestIndexOf,
TestIndexOfNextHighest, and TestIndexOfNextLowest with BDDHelper assertion
methods: use .ShouldBeEqualTo(expected) for numeric comparisons returned from
sut.IndexesOf(...).Single(), sut.IndexOfNextHighest(...), and
sut.IndexOfNextLowest(...), and use .ShouldBeFalse() for boolean checks (e.g.,
sut.IndexesOf(0.6F).Any().ShouldBeFalse()). Update each Assert.AreEqual(...)
instance in those three methods to the corresponding .ShouldBeEqualTo(...) or
.ShouldBeFalse() call on the actual value.
rwmcintosh
deleted the
1863-remove-check-for-monotonically-increasing-time-values
branch
3 participants
Fixes #1863 remove check for monotonically increasing time values?
Description
Relatively few fires created from what I can see.
Type of change
Please mark relevant options with an
xin the brackets.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Reviewer checklist
Mark everything that needs to be checked before merging the PR.
This change requires a documentation updateabove is selectedScreenshots (if appropriate):
Questions (if appropriate):
Summary by CodeRabbit
User Experience
Bug Fixes
Breaking Changes