-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve an AsList method for domains with stored GeneratorsOfDomain #3473
Improve an AsList method for domains with stored GeneratorsOfDomain #3473
Conversation
Let's see whether any of the manual tests fail due to new printing of e.g. |
dcc0a86
to
ea98daf
Compare
Codecov Report
@@ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there is a conflict with master now, due to another PR from you having been merged.
ea98daf
to
168907c
Compare
I resolved the merge conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really would feel better if some more people had a look at this (e.g. @stevelinton @ChrisJefferson ...)
if IsDuplicateFreeList( GeneratorsOfDomain( D ) ) then | ||
return GeneratorsOfDomain( D ); | ||
else | ||
return DuplicateFreeList( GeneratorsOfDomain( D ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm completely fine with changing the condition to
HasIsDuplicateFreeList( GeneratorsOfDomain( D ) ) and IsDuplicateFreeList( GeneratorsOfDomain( D ) )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the condition to
HasIsDuplicateFreeList( GeneratorsOfDomain( D ) ) and IsDuplicateFreeList( GeneratorsOfDomain( D ) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
168907c
to
eb8f36b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any potential harm with making this change as it exists now; there's essentially no overhead to the new check.
if IsDuplicateFreeList( GeneratorsOfDomain( D ) ) then | ||
return GeneratorsOfDomain( D ); | ||
else | ||
return DuplicateFreeList( GeneratorsOfDomain( D ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With HasIsDuplicateFreeList
it is of course fine.
The new method ensures that no copy of GeneratorsOfDomain is created if
GeneratorsOfDomain already is a duplicate free list.