Skip to content
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

This commit updates container_loops.jl to use the new iteration protocol #426

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

StephenVavasis
Copy link
Contributor

This commit rewrites container_loops.jl to incorporate the new iteration protocol
(introduced in 0.7.0-DEV) more thoroughly. It is not compatible with 0.6.

In addition, it fixes a bug in container_loops.jl in which the length()
function when applied to subranges (i.e., inclusive(a,b,c) or
exclusive(a,b,c)) returned the length of the whole container instead
of the length of the subrange. (There should be no value returned
for the length of the subrange since the data structure does not support
an O(1) algorithm or even O(log n) algorithm to compute the length.)

Some other smaller changes in this commit are as follows.

IntSet was changed to BitSet (name change in 0.7.0-DEV)
Small updates to documentation
Some assert statements in balanced_tree.jl that were present
during development are deleted.

(introduced in 0.7.0-DEV) more thoroughly. It is not compatible with 0.6.

In addition, it fixes a bug in container_loops.jl in which the length()
function when applied to subranges (i.e., inclusive(a,b,c) or
exclusive(a,b,c)) returned the length of the whole container instead
of the length of the subrange. (There should be no value returned
for the length of the subrange since the data structure does not support
an O(1) algorithm or even O(log n) algorithm to compute the length.)

Some other smaller changes in this commit are as follows.

    IntSet was changed to BitSet (name change in 0.7.0-DEV)
    Small updates to documentation
    Some assert statements in balanced_tree.jl that were present
    during development are deleted.
@StephenVavasis
Copy link
Contributor Author

I forgot to include in the commit message: this PR also rewrites test_sorted_containers.jl so that it runs much faster (10 sec versus 30+ sec).

@codecov
Copy link

codecov bot commented Aug 7, 2018

Codecov Report

Merging #426 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #426     +/-   ##
=========================================
- Coverage   98.19%   98.09%   -0.1%     
=========================================
  Files          30       30             
  Lines        1658     1573     -85     
=========================================
- Hits         1628     1543     -85     
  Misses         30       30
Impacted Files Coverage Δ
src/balanced_tree.jl 100% <100%> (ø) ⬆️
src/container_loops.jl 100% <100%> (ø) ⬆️
src/sorted_dict.jl 97.82% <0%> (-0.31%) ⬇️
src/sorted_multi_dict.jl 98.73% <0%> (-0.18%) ⬇️
src/circular_buffer.jl 100% <0%> (ø) ⬆️
src/tokens2.jl 100% <0%> (ø) ⬆️
src/sorted_set.jl 100% <0%> (ø) ⬆️
src/priorityqueue.jl 99.02% <0%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00831b0...8ac6dd4. Read the comment docs.

Copy link
Member

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I’m on my phone and can’t review the changes to test_sortrd_containers.jl

if p1 == p2
if i1a == t.tree[p1].child1
@assert(t.tree[p1].child2 == i2a || t.tree[p1].child3 == i2a)
# @assert(t.tree[p1].child2 == i2a || t.tree[p1].child3 == i2a)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you’ll ever uncomment these again? If not, then it’s better to just delete them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The curdepth commented out lines can be removed though. They are not asserts

@StephenVavasis
Copy link
Contributor Author

I won't ever uncomment those statements, but they do indicate invariants satisfied by the algorithm that may assist someone else in deciphering the code. Is there a better way to indicate (with a comment) a program invariant?

@StephenVavasis
Copy link
Contributor Author

Ok, I removed the commented-out asserts. The curdepth variable in that section of the code existed only to support the asserts, so I deleted the commented-out statements involving curdepth as well.

@kmsquire
Copy link
Member

I don’t know of a specific convention regarding invariants, but it would be nice to document them as such, to distinguish them from commented our code. Perhaps add Invariant: to the beginning of the line?

@oxinabox
Copy link
Member

I am sorry, I think I have miscommunicated.

I would not have deleted them.
I think for now best would be to
define a pair of macros:

@invariant_assert(expr)
and @invariant_supporting(expr).

Which for now just effectively comment out their contents.
But later we can add some kind of mode that enables them.
A bit like how @assert works in other languages,
except very disabled even in debug mode.

The other reason not to delete them is while we may later wish to inform the compiler of these facts later for optimisation purposes.
(or if not these facts, similar facts in other places. The principle stands)
So we really do want to keep them documented.

@StephenVavasis
Copy link
Contributor Author

OK, I backed out the last commit using git reset and changed the commented-out asserts to a new no-op macro as suggested. However, I don't know how to upload this commit to github. Below is the error message. Git is suggesting that I first pull, but of course I don't want to pull; I just want this latest commit to supersede the previous one. Any suggestions? I'm not well-versed in git.

c:\Users\vavasis\OneDrive - University of Waterloo\GitHub2\DataStructures.jl\src>git push origin container_loop_fixes
To https://github.com/JuliaCollections/DataStructures.jl
 ! [rejected]        container_loop_fixes -> container_loop_fixes (non-fast-forward)
error: failed to push some refs to 'https://github.com/JuliaCollections/DataStructures.jl'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@oxinabox
Copy link
Member

oxinabox commented Aug 22, 2018

I believe this calls for git push --force
Just like if you had rebased

@kmsquire
Copy link
Member

As @oxinabox said, git push --force is the correct thing to do in this case.

@kmsquire
Copy link
Member

kmsquire commented Sep 6, 2018

Bump. @StephanVavasis, is this good to go? Do you need to force push?

@StephenVavasis
Copy link
Contributor Author

Should be good to go... there are now no-op macros to indicate program invariants.

@kmsquire kmsquire merged commit 1cb2535 into master Sep 11, 2018
@StephenVavasis StephenVavasis mentioned this pull request Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants