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.

Sign up for GitHub

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

Jump to bottom

Introduce new dropunusedlevels! function and allow using from map #43

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Introduce new dropunusedlevels! function and allow using from map #43

wants to merge 2 commits into from

Commits on Nov 21, 2020

  1. JuliaFormatter formatting

    @quinnj
    quinnj committed Nov 21, 2020
    Configuration menu
    Copy the full SHA
    87ab96b View commit details
    Browse the repository at this point in the history

  2. Introduce new dropunusedlevels! function and allow using from map 

    Addresses #36. This PR proposes ways to address the issue in #36, namely
that unused pool levels can cause unexpected results when mapping (and
possibly other operations). It introduces a new exported
`dropunusedlevels!` function that will scan a `PooledArray` and remove
any pooled values not being reference in the ref array any more. It also
updates the `map` implementation to allow a keyword argument
`dropunusedlevels=true` to call this before mapping. We could perhaps
consider making that `true` by default to avoid the original issue
happening, though it comes with a performance cost of checking for
unused levels.

This PR also updates the implementation of `map`; it was previously
doing quite a bit of extra work: mapping over keys twice for some
reason, in addition to checking the resulting mapped values for
duplicates. As far as I can tell/understand, having duplicates in the
pool shouldn't actually cause any real issues, but maybe others can
comment more on that. The only place I can think is maybe in the sorting
code which I admit I haven't look at very closely.

Anyway, still need to add tests here, but figured I would put up the PR
as-is for discussion on the approach.
    @quinnj
    quinnj committed Nov 21, 2020
    Configuration menu
    Copy the full SHA
    769d22d View commit details
    Browse the repository at this point in the history