Fix scalar * and / mutating the Solution in place (fixes #457) - #461
Open
youdie006 wants to merge 1 commit into
Open
Fix scalar * and / mutating the Solution in place (fixes #457)#461youdie006 wants to merge 1 commit into
youdie006 wants to merge 1 commit into
Conversation
Standalone sol * factor and sol / factor mutated the original Solution and returned the same object. __mul__/__truediv__ did self.volume *= factor; return self, and the volume setter rescales every component in place, so sol * 2 doubled sol's volume and moles and returned an alias ((sol * 2) is sol was True) instead of a new scaled Solution. __mul__/__truediv__ now return a new scaled Solution, built via the existing faithful from_dict(as_dict(...)) round-trip and then scaling the copy's volume; the original is left unchanged. __imul__/__itruediv__ are added to preserve the documented in-place *= / /= scaling behavior (mutate self, return self), and __rmul__ is added so factor * sol works. This makes * and / consistent with Python numeric-type semantics while keeping in-place *= / /= intact. Fixes KingsburyLab#457.
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
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.
Fixes #457.
Problem
Standalone
sol * factorandsol / factormutated the original Solution and returned the same object. Root cause:__mul__/__truediv__didself.volume *= factor; return self, and thevolumesetter rescales every component in place -- sosol * 2doubledsol's volume and moles and returned an alias ((sol * 2) is solwasTrue), instead of a new scaled Solution. Reported by @jjstickel.Fix
__mul__/__truediv__now return a new scaled Solution, built via the existing faithfulfrom_dict(as_dict(...))round-trip and then scaling the copy's volume; the original is left unchanged.__imul__/__itruediv__are added to preserve the documented in-place*=//=scaling behavior (mutate self, return self).__rmul__is added sofactor * solworks (commutativity).This makes
*and/consistent with Python numeric-type semantics, as @rkingsbury requested in the issue thread, while keeping the documented in-place*=//=behavior intact.Tests
test_multiplication_returns_new_solution: assertssol * 2 is not sol, the original is unchanged, the returned Solution has 2x volume/moles,2 * solworks (rmul),sol / 2returns a new halved Solution, andsol *= 3/sol /= 3still mutate in place and return self.assert doubled is not sol) and the fulltests/test_solution.pysuite passes with no regressions (test_arithmetic_and_copyincluded).This contribution was prepared with AI assistance and reviewed by me before submission.