Improve an AsList method for domains with stored GeneratorsOfDomain #3473

ssiccha · 2019-05-24T11:41:18Z

The new method ensures that no copy of GeneratorsOfDomain is created if
GeneratorsOfDomain already is a duplicate free list.

ssiccha · 2019-05-24T11:42:35Z

Let's see whether any of the manual tests fail due to new printing of e.g. AsList(Domain([1 .. 3])).

coveralls · 2019-05-24T12:33:04Z

Coverage increased (+0.0008%) to 85.169% when pulling eb8f36b on ssiccha:ss/improve-AsList-for-domains into 5d81b55 on gap-system:master.

lib/domain.gi

codecov · 2019-05-26T14:05:23Z

Codecov Report

Merging #3473 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3473      +/-   ##
==========================================
- Coverage   85.34%   85.34%   -0.01%     
==========================================
  Files         699      699              
  Lines      346504   346487      -17     
==========================================
- Hits       295733   295715      -18     
- Misses      50771    50772       +1

Impacted Files	Coverage Δ
lib/domain.gi	`87.93% <100%> (-2.57%)`	⬇️
lib/coll.gd	`95.09% <0%> (-0.07%)`	⬇️
lib/domain.gd	`100% <0%> (ø)`	⬆️
lib/grp.gi	`89.36% <0%> (+0.07%)`	⬆️

fingolfin

Unfortunately, there is a conflict with master now, due to another PR from you having been merged.

lib/domain.gi

ssiccha · 2019-05-30T07:45:24Z

I resolved the merge conflict.

fingolfin

I really would feel better if some more people had a look at this (e.g. @stevelinton @ChrisJefferson ...)

fingolfin · 2019-05-30T10:04:27Z

lib/domain.gi

+        if IsDuplicateFreeList( GeneratorsOfDomain( D ) ) then
+            return GeneratorsOfDomain( D );
+        else
+            return DuplicateFreeList( GeneratorsOfDomain( D ) );


Thinking about this some more, I am not really sure anymore whether this is an "improvement", as the description of this PR claims: AsList is an attribute. So we are talking about a one-time cost here. Now, with the old code, we'd just call DuplicateFreeList, which performs an O(n^2) algorithm.
With this "improvement", we instead first run IsDuplicateFreeList, which also is O(n^2). And if it fails, we still run DuplicateFreeList. So while there may be scenarios where this saves something (namely memory, but not performance), there are also scenarios where it results in a slow-down.

Are there any real-world applications that motivate this change?

Oh, I'm completely fine with changing the condition to
HasIsDuplicateFreeList( GeneratorsOfDomain( D ) ) and IsDuplicateFreeList( GeneratorsOfDomain( D ) ).

Are there any real-world applications that motivate this change?

I want AsList(Domain([1..3])) to return the range [1..3]. At the moment gap returns the plain list [1,2,3]. As the range [1..3] knows that it is a IsDuplicateFreeList in this situation there should be no reason to copy it and create a new plain list.

I changed the condition to
HasIsDuplicateFreeList( GeneratorsOfDomain( D ) ) and IsDuplicateFreeList( GeneratorsOfDomain( D ) )

With HasIsDuplicateFreeList it is of course fine.

... stored GeneratorsOfDomain. The new version ensures that no copy of GeneratorsOfDomain is created if GeneratorsOfDomain already is a duplicate free list. Adds tests for this situation.

wilfwilson

I don't see any potential harm with making this change as it exists now; there's essentially no overhead to the new check.

fingolfin · 2019-06-05T20:55:11Z

lib/domain.gi

+        if IsDuplicateFreeList( GeneratorsOfDomain( D ) ) then
+            return GeneratorsOfDomain( D );
+        else
+            return DuplicateFreeList( GeneratorsOfDomain( D ) );


With HasIsDuplicateFreeList it is of course fine.

ssiccha added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 24, 2019

ssiccha changed the title ~~lib: improve an AsList method for domains with ...~~ Improve an AsList method for domains with stored GeneratorsOfDomain May 24, 2019

fingolfin reviewed May 24, 2019

View reviewed changes

lib/domain.gi Outdated Show resolved Hide resolved

ssiccha force-pushed the ss/improve-AsList-for-domains branch 2 times, most recently from dcc0a86 to ea98daf Compare May 26, 2019 14:05

ssiccha mentioned this pull request May 27, 2019

WIP: Add IsDirectProductDomain objects and related basic methods #3477

Closed

fingolfin reviewed May 28, 2019

View reviewed changes

lib/domain.gi Outdated Show resolved Hide resolved

ssiccha force-pushed the ss/improve-AsList-for-domains branch from ea98daf to 168907c Compare May 30, 2019 07:45

fingolfin approved these changes May 30, 2019

View reviewed changes

fingolfin self-requested a review May 30, 2019 10:04

fingolfin requested changes May 30, 2019

View reviewed changes

lib: improve an AsList method for domains with ...

eb8f36b

... stored GeneratorsOfDomain. The new version ensures that no copy of GeneratorsOfDomain is created if GeneratorsOfDomain already is a duplicate free list. Adds tests for this situation.

ssiccha force-pushed the ss/improve-AsList-for-domains branch from 168907c to eb8f36b Compare May 30, 2019 10:43

wilfwilson approved these changes Jun 5, 2019

View reviewed changes

fingolfin approved these changes Jun 5, 2019

View reviewed changes

fingolfin merged commit 48612ec into gap-system:master Jun 5, 2019

ssiccha deleted the ss/improve-AsList-for-domains branch August 7, 2019 08:47

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019

olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve an AsList method for domains with stored GeneratorsOfDomain #3473

Improve an AsList method for domains with stored GeneratorsOfDomain #3473

ssiccha commented May 24, 2019 •

edited

Loading

ssiccha commented May 24, 2019

coveralls commented May 24, 2019 •

edited

Loading

codecov bot commented May 26, 2019 •

edited

Loading

fingolfin left a comment

ssiccha commented May 30, 2019

fingolfin left a comment

fingolfin May 30, 2019

ssiccha May 30, 2019

ssiccha May 30, 2019 •

edited

Loading

ssiccha May 30, 2019

fingolfin Jun 5, 2019

wilfwilson left a comment

fingolfin Jun 5, 2019

Improve an AsList method for domains with stored GeneratorsOfDomain #3473

Improve an AsList method for domains with stored GeneratorsOfDomain #3473

Conversation

ssiccha commented May 24, 2019 • edited Loading

ssiccha commented May 24, 2019

coveralls commented May 24, 2019 • edited Loading

codecov bot commented May 26, 2019 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

ssiccha commented May 30, 2019

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin May 30, 2019

Choose a reason for hiding this comment

ssiccha May 30, 2019

Choose a reason for hiding this comment

ssiccha May 30, 2019 • edited Loading

Choose a reason for hiding this comment

ssiccha May 30, 2019

Choose a reason for hiding this comment

fingolfin Jun 5, 2019

Choose a reason for hiding this comment

wilfwilson left a comment

Choose a reason for hiding this comment

fingolfin Jun 5, 2019

Choose a reason for hiding this comment

ssiccha commented May 24, 2019 •

edited

Loading

coveralls commented May 24, 2019 •

edited

Loading

codecov bot commented May 26, 2019 •

edited

Loading

ssiccha May 30, 2019 •

edited

Loading