Explicitly handle NAN values #539
Open
+265
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/property/types/CloudWrapperFloat.h:45
- [nitpick] For consistency with the usage of std::isnan and clarity, consider using std::fabs instead of the bare fabs. This ensures the correct overload is used for floating-point values.
return _primitive_value != _cloud_value && fabs(_primitive_value - _cloud_value) >= Property::_min_delta_property;
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 94.81% 94.91% +0.09%
==========================================
Files 31 32 +1
Lines 1370 1376 +6
==========================================
+ Hits 1299 1306 +7
+ Misses 71 70 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
per1234
added
type: imperfection
|
Personally, I would not merge this PR, since the standard IEEE754 assigns different meanings to NaN values. This will stop floating point exceptions to propagate to cloud thus making us not adhering with the standard. Can you provide me with a valid explanation as to why we need to merge this? If the issue is with the communication with cloud, I think the problem should be fixed on that side, to allow the propagation of those values. |
|
@andreagilardoni NaN is a valid float value and the current implementation of isDifferentFromCloud yields incorrect behavior in presence of this value. I don't see how that could be desirable. |
|
@sebromero can you describe to me the incorrect behavior we have? and how can I replicate it? |
|
Ciao @andreagilardoni ! Sure, I'll explain. In any case I don't understand what you mean by floating point exceptions propagating to cloud? This affected function just returns a boolean and Arduino has no exception handling, so I'm not sure what would propagate anywhere 🤔 If you mean that a local NAN value should propagate to the cloud, then I totally agree and this is exactly what this PR fixes. So, the job of #include <iostream>
#include <cmath>
bool isDifferentFromCloud(float _primitive_value, float _cloud_value, float _min_delta_property = 0.0f) {
bool isEqual = _primitive_value == _cloud_value;
std::cout << "Is equal " << _primitive_value << " == " << _cloud_value << ": " << isEqual << std::endl;
int difference = abs(_primitive_value - _cloud_value);
std::cout << "Difference " << _primitive_value << " - " << _cloud_value << ": " << difference << std::endl;
bool isDifferent = _primitive_value != _cloud_value && difference >= _min_delta_property;
std::cout << "Is different " << _primitive_value << " != " << _cloud_value << ": " << isDifferent << std::endl;
std::cout << "-------------------" << std::endl;
return isDifferent;
// Source: return _primitive_value != _cloud_value && (abs(_primitive_value - _cloud_value) >= Property::_min_delta_property);
}
int main(){
// Correct
isDifferentFromCloud(1.5f, 1.0f); // Expected result: true, actual result: true
isDifferentFromCloud(1.0f, 1.5f); // Expected result: true, actual result: true
isDifferentFromCloud(1.5f, 1.5f); // Expected result: false, actual result: false
isDifferentFromCloud(NAN, NAN); // Expected result: false, actual result: false (only because NAN != NAN)
// Broken
isDifferentFromCloud(1.5f, 1.0f, 0.25f); // Expected result: true, actual result: false
isDifferentFromCloud(1.0f, 1.5f, 0.25f); // Expected result: true, actual result: false
isDifferentFromCloud(NAN, 1.5f); // Expected result: true, actual result: false
isDifferentFromCloud(1.5f, NAN); // Expected result: true, actual result: false
return 0;
} |
|
Memory usage change @ e3245de
Click for full report table
Click for full report CSV |
andreagilardoni
force-pushed
the
handle-nan
branch
from
ba0da0c to
a2dad01
Compare
|
Memory usage change @ 67e1d8d
Click for full report table
Click for full report CSV |
|
Memory usage change @ cb5ac03
Click for full report table
Click for full report CSV |
|
Memory usage change @ 6fef724
Click for full report table
Click for full report CSV |
andreagilardoni
force-pushed
the
handle-nan
branch
from
6fef724 to
9c2aaef
Compare
|
Test.zip |
andreagilardoni
approved these changes
Apr 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the current version NAN values are not handled. This leads to unexpected behaviour since the numerical values of both the local variable and the cloud version are used to determine if there is a difference. Without this change a local variable that is initialized with a default of NAN will never receive the cloud value if the permission is set to
Write. If the permission is set toReadWritethe cloud value will always be overridden with NAN even withonSync(CLOUD_WINS).This PR explicitly handles the cases where either one of the values is NAN (non-equal), or both are NAN (equal).