Don't use fast method for composing with identity mapping if source & range don't match #2533

stevelinton · 2018-06-08T14:19:37Z

The code that was there could produce a composition whose range didn't match the second argument, which can never be right.

stevelinton · 2018-06-08T14:22:25Z

Needs "Squash and merge" when the time comes.

codecov · 2018-06-08T18:23:50Z

Codecov Report

Merging #2533 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2533      +/-   ##
==========================================
+ Coverage    74.4%    74.4%   +<.01%     
==========================================
  Files         481      481              
  Lines      243488   243486       -2     
==========================================
- Hits       181157   181156       -1     
+ Misses      62331    62330       -1

Impacted Files	Coverage Δ
lib/mapprep.gi	`69.37% <100%> (+0.08%)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`63.93% <0%> (-0.77%)`	⬇️
src/iostream.c	`63.77% <0%> (+1.13%)`	⬆️

stevelinton · 2018-06-10T08:09:53Z

Probably also needs a test that works in master. I discovered this with #2521. Adding "Do Not Merge" for now.

fingolfin · 2018-06-11T12:26:05Z

tst/testbugfix/2018-06-11-mapping.tst

@@ -0,0 +1,13 @@
+#
+# There was a bug where composition of an identify mapping and another mapping x where the Source of the identity mapping contained the


identify -> identity

Also, could you please wrap your lines after 80 or so characters?

fingolfin · 2018-06-11T12:26:30Z

tst/testbugfix/2018-06-11-mapping.tst

+#
+# There was a bug where composition of an identify mapping and another mapping x where the Source of the identity mapping contained the
+# ImagesSource of x, but not its whole range, simply returned x, resulting in a compoition whose Range was strictly bigger than that
+# of its first argument, and causign problems in IsomorphismPermGroup


causign -> causing

fingolfin · 2018-06-11T12:27:33Z

tst/testbugfix/2018-06-11-mapping.tst

@@ -0,0 +1,13 @@
+#
+# There was a bug where composition of an identify mapping and another mapping x where the Source of the identity mapping contained the
+# ImagesSource of x, but not its whole range, simply returned x, resulting in a compoition whose Range was strictly bigger than that


compoition -> composition

fingolfin · 2018-06-11T12:37:32Z

tst/testbugfix/2018-06-11-mapping.tst

+gap> psi := IdentityMapping(SymmetricGroup(6));
+IdentityMapping( Sym( [ 1 .. 6 ] ) )
+gap> Range(CompositionMapping(psi,phi));
+Sym( [ 1 .. 6 ] )


Hmm, phi has Sym(7) as its range (but not as its image!), so on a purely formal level, it shouldn't compose with psi like that, should it?

But OK, GAP doesn't seem to specify what it allows for composition, so I'll assume that only the image is relevant for the legality of the composition, not the range. I guess being stricter about this might be annoying in some situations, while the added "type safety" it provides may not be worth it. (Also, changing existing behavior like that of course is risky)

@fingolfin -- the manual is rather silent on what is allowed for the sources and ranges in a composition, whether there is an implicit restriction, and so on. I'll flag an issue to document and test some set of rules, but I don't want it to get tangled in this, which seems to be a safe bugfix.

Thanks, that's fine by me. So I'll just wait for the typos in the comments above to be fixed.

… range don't match

codecov · 2018-06-11T19:55:54Z

Codecov Report

Merging #2533 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2533      +/-   ##
==========================================
+ Coverage    74.4%    74.4%   +<.01%     
==========================================
  Files         481      481              
  Lines      243488   243486       -2     
==========================================
  Hits       181157   181157              
+ Misses      62331    62329       -2

Impacted Files	Coverage Δ
lib/mapprep.gi	`69.37% <100%> (+0.08%)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`64.19% <0%> (-0.52%)`	⬇️
src/iostream.c	`63.77% <0%> (+1.13%)`	⬆️

hulpke

So just returning the other map (if one is the identity) is clearly wrong, so the code has to be changed, which is good. There is a chance of a dual problem with the multiplication "the other way round". Also, it would be worth checking what "next method" then gets called, to ensure that this does not lead to performance problems.

stevelinton · 2018-06-12T08:14:48Z

@hulpke The dual code (which is just a few lines further up the same file) doesn't have the same problem, which is in and of itself, curious. The "next method" in general, produces an object in IsCompositionMappingRep. One could, I suppose have (in at least one of the two orders) a method that delegates to RestrictedMapping (is there an obvious reason why there isn't a RangeRestrictedMapping which would handle the other case? I don't know if it would make any real difference. We really need to sort out the more general questions in #2540 first.

hulpke · 2018-06-12T19:25:26Z

@stevelinton Thanks for checking. Yes, Images and Ranges are a mess and we might need to keep an eye on this area.

stevelinton added kind: bug Issues describing general bugs, and PRs fixing them topic: library labels Jun 8, 2018

stevelinton mentioned this pull request Jun 8, 2018

Reorder methods after new implications are added #2521

Closed

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

stevelinton added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jun 10, 2018

stevelinton force-pushed the mapping-range branch 3 times, most recently from 76b5ba0 to a2dbe3e Compare June 11, 2018 12:17

stevelinton removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jun 11, 2018

stevelinton requested a review from hulpke June 11, 2018 12:18

fingolfin requested changes Jun 11, 2018

View reviewed changes

Don't use fast method for composing with identity mapping if source &…

470a3ee

… range don't match

stevelinton force-pushed the mapping-range branch from a2dbe3e to 470a3ee Compare June 11, 2018 14:55

fingolfin approved these changes Jun 11, 2018

View reviewed changes

hulpke reviewed Jun 12, 2018

View reviewed changes

stevelinton added a commit to stevelinton/gap that referenced this pull request Jun 12, 2018

Include gap-system#2533 for convenience

c5c327e

fingolfin merged commit fff8dcb into gap-system:master Jun 12, 2018

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use fast method for composing with identity mapping if source & range don't match #2533

Don't use fast method for composing with identity mapping if source & range don't match #2533

stevelinton commented Jun 8, 2018

stevelinton commented Jun 8, 2018

codecov bot commented Jun 8, 2018 •

edited

Loading

stevelinton commented Jun 10, 2018

fingolfin Jun 11, 2018

fingolfin Jun 11, 2018

fingolfin Jun 11, 2018

fingolfin Jun 11, 2018

stevelinton Jun 11, 2018

fingolfin Jun 11, 2018

codecov bot commented Jun 11, 2018 •

edited

Loading

hulpke left a comment

stevelinton commented Jun 12, 2018

hulpke commented Jun 12, 2018

		@@ -0,0 +1,13 @@
		#
		# There was a bug where composition of an identify mapping and another mapping x where the Source of the identity mapping contained the

Don't use fast method for composing with identity mapping if source & range don't match #2533

Don't use fast method for composing with identity mapping if source & range don't match #2533

Conversation

stevelinton commented Jun 8, 2018

stevelinton commented Jun 8, 2018

codecov bot commented Jun 8, 2018 • edited Loading

Codecov Report

stevelinton commented Jun 10, 2018

fingolfin Jun 11, 2018

Choose a reason for hiding this comment

fingolfin Jun 11, 2018

Choose a reason for hiding this comment

fingolfin Jun 11, 2018

Choose a reason for hiding this comment

fingolfin Jun 11, 2018

Choose a reason for hiding this comment

stevelinton Jun 11, 2018

Choose a reason for hiding this comment

fingolfin Jun 11, 2018

Choose a reason for hiding this comment

codecov bot commented Jun 11, 2018 • edited Loading

Codecov Report

hulpke left a comment

Choose a reason for hiding this comment

stevelinton commented Jun 12, 2018

hulpke commented Jun 12, 2018

codecov bot commented Jun 8, 2018 •

edited

Loading

codecov bot commented Jun 11, 2018 •

edited

Loading