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

Sorted containers -> new iteration protocol #384

Conversation

StephenVavasis
Copy link
Contributor

On branch newiterationsortedcontainers
Changes to be committed:
modified: ../docs/src/sorted_containers.md
modified: DataStructures.jl
modified: balanced_tree.jl
modified: container_loops.jl
modified: ../test/test_sorted_containers.jl

This commit updates container_loops.jl to use the new iteration protocol
(introduced in 0.7.0-DEV). It should be backwards compatible with 0.6.2.

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

The CI tests on my pull request are failing because of the ambiguity-detector is detecting ambiguities in 0.7. However, these ambiguities are being caused by source files other than the ones modified in this PR.

@StephenVavasis
Copy link
Contributor Author

Question for the experts: Is there a good way to test that the new definitions in my PR for IteratorEltype and IteratorSize are correct? I didn't know what to put in the test set to test these functions.

@kmsquire
Copy link
Member

kmsquire commented May 22, 2018 via email

@codecov
Copy link

codecov bot commented May 25, 2018

Codecov Report

Merging #384 into master will decrease coverage by 0.52%.
The diff coverage is 87.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
- Coverage   96.59%   96.07%   -0.53%     
==========================================
  Files          31       31              
  Lines        2318     2367      +49     
==========================================
+ Hits         2239     2274      +35     
- Misses         79       93      +14
Impacted Files Coverage Δ
src/DataStructures.jl 100% <ø> (ø) ⬆️
src/balanced_tree.jl 100% <100%> (ø) ⬆️
src/container_loops.jl 89.65% <87.5%> (-9.23%) ⬇️

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 a8d9477...43425a7. Read the comment docs.

@StephenVavasis
Copy link
Contributor Author

The CI checks are now passed, but the coverage has dropped. One reason the coverage dropped is because I didn't test the new length and eltype methods, so I'm adding more tests for these now. However, the coverage will probably be depressed because of the branch on which version in container_loops.jl, unless the coverage tool merges coverage from 0.6 and 0.7. (does it?)

@StephenVavasis
Copy link
Contributor Author

As an aside: I know Base has some undocumented infrastructure for iterating over keys (e.g., Base.Keyset) which is not used in container_loops.jl. What would be the advantage of using this infrastructure? Is it required for compatibility with 0.7?

 On branch newiterationsortedcontainers
 Changes to be committed:
	modified:   ../docs/src/sorted_containers.md
	modified:   DataStructures.jl
	modified:   balanced_tree.jl
	modified:   container_loops.jl
	modified:   ../test/test_sorted_containers.jl

This commit updates container_loops.jl to use the new iteration protocol
(introduced in 0.7.0-DEV). It should be backwards compatible with 0.6.2.

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.

Add more tests for new length and eltype methods
@StephenVavasis
Copy link
Contributor Author

I inserted additional tests in test_sorted_containers.jl for new eltype and length methods in order to improve coverage. The testing in fact uncovered bugs, so I also revised container_loops.jl again. But the coverage still seems to be low, so before this is merged, I'd like to test the coverage locally. Is the GitHub coverage tool the same as Coverage.jl? Is the tool run in Julia 0.6 or 0.7 or both?

@tpapp
Copy link

tpapp commented Aug 2, 2018

Since #28365 introduced deprecation messages, it would be great to get this merged.

@StephenVavasis
Copy link
Contributor Author

I can try to get back to work on this. I just looked at source file DataStructures.jl in the master branch. It still imports start, done, next but not iterate. I would have presumed that someone else aside from this PR has updated that file since May. Is there another branch besides master and this PR where there is already work on the new iteration protocol?

@StephenVavasis
Copy link
Contributor Author

Withdrawn-- superseded by #426 .

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