You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Floor.mutate() was applying transforms by undoing the previous mutation (reverse angle, inverse scale) and then applying the new one.
This approach is not a true inverse — the undo math is order-dependent, and floating-point compound roundings on every call.
The result is that calling mutate() multiple times on the same floor progressively incorrect geometry, a problem for ML data augmentation pipelines that generate multiple crops at different angles.
Changes
MLStructFP/db/__init__.py MLStructFP/db/_floor.py
Added _original_points: Optional[Dict] field to Floor, initialized to None in __init__.
On the first mutate() call, the original (x, y) of every point across all components is snaps hotted (?) into _original_points, assigned by object identity.
On every further call, points are restored from the snapshot before the transform is applied, so each subsequent mutation is always computed from a clean, exact baseline with no accumulated error.
Removed the old undo logic (self.mutate(-_angle, 1/_sx, 1/_sy, ...)), which was the source of the bug; after testing.
No breaking API changes.
Testing
A standalone test file (test_floor_mutate.py) is available in case for addition to the repository, the results are the following:
17 tests, all passing.
test_identity_is_noop (__main__.TestMutateBasic.test_identity_is_noop) ... ok
test_rotate_180 (__main__.TestMutateBasic.test_rotate_180) ... ok
test_rotate_90 (__main__.TestMutateBasic.test_rotate_90) ... ok
test_scale_only (__main__.TestMutateBasic.test_scale_only) ... ok
test_double_call_same_angle_no_drift (__main__.TestMutateIdempotency.test_double_call_same_angle_no_drift) ... ok
test_fifty_calls_no_drift (__main__.TestMutateIdempotency.test_fifty_calls_no_drift) ... ok
test_sequential_different_mutations_resolve_from_origin (__main__.TestMutateIdempotency.test_sequential_different_mutations_resolve_from_origin) ... ok
test_identity_after_mutation_restores_original (__main__.TestMutateRestore.test_identity_after_mutation_restores_original) ... ok
test_second_scale_applies_to_original_not_first (__main__.TestMutateRestore.test_second_scale_applies_to_original_not_first) ... ok
test_scale_first_vs_rotate_first_differ_nonuniform (__main__.TestMutateScaleOrder.test_scale_first_vs_rotate_first_differ_nonuniform) ... ok
test_uniform_scale_order_irrelevant (__main__.TestMutateScaleOrder.test_uniform_scale_order_irrelevant) ... ok
test_bb_cache_invalidated (__main__.TestMutateState.test_bb_cache_invalidated) ... ok
test_mutator_properties_after_mutate (__main__.TestMutateState.test_mutator_properties_after_mutate) ... ok
test_mutator_properties_before_mutation (__main__.TestMutateState.test_mutator_properties_before_mutation) ... ok
test_snapshot_created_on_first_call (__main__.TestMutateState.test_snapshot_created_on_first_call) ... ok
test_snapshot_not_overwritten_on_subsequent_calls (__main__.TestMutateState.test_snapshot_not_overwritten_on_subsequent_calls) ... ok
test_snapshot_values_match_pre_mutation_coordinates (__main__.TestMutateState.test_snapshot_values_match_pre_mutation_coordinates) ...
ok
----------------------------------------------------------------------
Ran 17 tests in 0.004s
OK
1. Fix some broken tests
2. It is not necessary to refer, in function docs, to "Unlike the previous implementation".
I think function/class docs should focus on the present, comments on past & present, and TODOs for the future.
Will fix, regardless, since I see that you have experience and on the same location do you mind if I email you with unrelated questions regarding academia and code please? Thank you in advance Pablo.
1. Fix some broken tests
2. It is not necessary to refer, in function docs, to "Unlike the previous implementation".
I think function/class docs should focus on the present, comments on past & present, and TODOs for the future.
Will fix, regardless, since I see that you have experience and on the same location do you mind if I email you with unrelated questions regarding academia and code please? Thank you in advance Pablo.
Hi @fcarvajalbrown , sure feel free to email me for these reasons.
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
2 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.
Overview
Floor.mutate()was applying transforms by undoing the previous mutation (reverse angle, inverse scale) and then applying the new one.This approach is not a true inverse — the undo math is order-dependent, and floating-point compound roundings on every call.
The result is that calling
mutate()multiple times on the same floor progressively incorrect geometry, a problem for ML data augmentation pipelines that generate multiple crops at different angles.Changes
MLStructFP/db/__init__.pyMLStructFP/db/_floor.py_original_points: Optional[Dict]field toFloor, initialized toNonein__init__.mutate()call, the original(x, y)of every point across all components is snaps hotted (?) into_original_points, assigned by object identity.self.mutate(-_angle, 1/_sx, 1/_sy, ...)), which was the source of the bug; after testing.Testing
A standalone test file (
test_floor_mutate.py) is available in case for addition to the repository, the results are the following:17 tests, all passing.