--[Bugfix] - Compare Configuration scalar doubles properly#2478
Merged
--[Bugfix] - Compare Configuration scalar doubles properly#2478
Conversation
This uses the same mechanism that Magnum constructs use, so results are consistent
There is a possibility, albeit remote, that 2 otherwise identical configurations might have a different number of internal-use/hidden values. Note, these are only accessible internally (in c++ source) and so the likelihood of this happening is vanishingly small.
facebook-github-bot
added
the
CLA Signed
11 tasks
aclegg3
approved these changes
Sep 30, 2024
Contributor
aclegg3
left a comment
aclegg3
left a comment
There was a problem hiding this comment.
LGTM, double makes sense for python APIs particularly. Any other types you predict will need to be explicitly marked (which we may ever use)?
Contributor
Author
We would probably need extend the support if we ever want to differentiate (type-wise) between different size floating points (i.e. add explicit 32bit float support) or if we wanted to support non-magnum floating-point-based numeric constructs. |
jturner65
added a commit
to jturner65/habitat-sim
that referenced
this pull request
Oct 29, 2024
…esearch#2478) * --remove unnecessary magnum decorator * --add and test support for scalar ConfigValues requiring fuzzy compare This uses the same mechanism that Magnum constructs use, so results are consistent * --separate Configuration fuzzy compare tests; minor updates * --make sure comparisons only involve the number of non-hidden values There is a possibility, albeit remote, that 2 otherwise identical configurations might have a different number of internal-use/hidden values and would appear to be different when they were, in fact, the same. Not anymore. Note, these are only accessible internally (in c++ source) and so the likelihood of this happening is vanishingly small.
jturner65
added a commit
that referenced
this pull request
Nov 4, 2024
* --[Bugfix] - Scene Reset Redundancy (#2470) * --modify reset() to not perform redundant object re-placement After successful scene creation, reset is called, but the objects have all already been placed in their initial positions. * --fix bindings Don't expose the boolean in reset to python, should only be consumed from Simulator::reconfigure * --[BE] - Add ability to filter Configuration subconfigs (#2471) * --add subconfig filtering process. This function will remove any values and subconfigs from a Configuration that match those found within a passed Configuration. * --minor naming clarification * --expand Configuration tests to test filtering; rename test Test only tests Configurations, so name appropriately * --[Bugfix] - Address ObjectInstanceAttributes save issues (#2472) * --access init instance attributes for an rigid/AO via const ref or copy * --access sim directly to query for defaultCOMCorrection state So that objects added to an existing scene will use the correct defaultCOMCorrection state. * --verify requested type is appropriate * --add queries for current state of Configuration fields Query whether a field is a default value, a hidden value or backed by an enum (i.e. 'translated') * --properly set enum-backed/'translated' string fields in instance attrs * --add is_visible boolean for AOs For instance consistency - not yet supported. * --only set instance vals if not default config vals; clean up JSON write Configuration now supports not writing initialization/default fields to JSON. * --Add access to user_defined view and setter * --better names for object init attrs and object instance init attrs * --use base obj's current state to build ObjectInstanceAttr and obj copy * --only change fields if they are different than defaults. * --[BE] - Baselink ID init via constant (#2465) * --system-wide constant for baseLink ID * --use BASELINK_ID instead of magic number -1 to denote baseLink * --Use ID_UNDEFINED for rigid object link ids. * --more fixes * --[BE] - ManagedContainer updates and cleanups (#2475) * --clean up handle query * --move object library map settings to base class. * --make managed container map member variables private. * --verify templated accessor results are cast to legitimate type. * --simplify caller-message arguments * --clarify naming of existing dirty flag, in preparation of file-save-based flag. * --move filepath check flag to base attributes to make available * --treat pbr/ibl helper key as hidden field * --provide rudiments of file-status dirty flags. The purpose of this flag is to specify that a particular Managed File-based object has been registered in its Manager in a state that is different than that of its disk-based counterpart. * --rename constructor-building method for clarity * --[Bugfix] - Compare Configuration scalar doubles properly (#2478) * --remove unnecessary magnum decorator * --add and test support for scalar ConfigValues requiring fuzzy compare This uses the same mechanism that Magnum constructs use, so results are consistent * --separate Configuration fuzzy compare tests; minor updates * --make sure comparisons only involve the number of non-hidden values There is a possibility, albeit remote, that 2 otherwise identical configurations might have a different number of internal-use/hidden values and would appear to be different when they were, in fact, the same. Not anymore. Note, these are only accessible internally (in c++ source) and so the likelihood of this happening is vanishingly small. * add a check, error meesage, and correction for 0 scale in box shapes (#2464) Co-authored-by: aclegg3 <alexanderwclegg@gmail.com> --------- Co-authored-by: John Turner <7strbass@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation and Context
This PR adds, and tests, proper fuzzy comparison between scalar ConfigValues that require it (i.e. doubles) using the same Magnum mechanism that supported Magnum floating point vector-based types use. This comparison uses a scaled epsilon value that acts as a threshold between two values being equal or not. See here for comparison details.
Support for fuzzy comparison must be implemented (following the pattern here for doubles) for any numeric types added to the ConfigValue supported list only if they do not use a similar comparison mechanism already (i.e. this does not need to be included for any magnum floating-point types)
How Has This Been Tested
Locally c++ and python. Tests have been added to validate the process for both below-threshold and above-threshold values.
Types of changes
Checklist