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

Add clear-cache #60

Merged
merged 9 commits into from
Dec 11, 2020
Merged

Add clear-cache #60

merged 9 commits into from
Dec 11, 2020

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented Nov 30, 2020

This PR is an attempt to implement clearing of the cache (#14).

EDIT: In a previous version of this comment, I stated that other types than IdDict needed to be considered. Since Base.empty! takes collections, I believe that the code should now work for any relevant type.

src/Memoize.jl Outdated Show resolved Hide resolved
src/Memoize.jl Outdated Show resolved Hide resolved
src/Memoize.jl Outdated Show resolved Hide resolved
Co-authored-by: Cédric St-Jean <cedric.stjean@gmail.com>
@rikhuijzer
Copy link
Contributor Author

It seems that this PR is not making progress. Is there anything I can do?

@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

If you do

module A
@memoize foo(x) = x
end

using A: foo

@clear_memoize_cache foo

I don't believe it would work, right? I'm sorry that I didn't think of it earlier.

I think that clear_memoize_cache!(foo) would be the better approach. That means you need to implement Memoize.clear_memoize_cache(::typeof(foo)) in the @memoize expansion.

@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

Or even better, empty!(memoize_cache(foo)).

@rikhuijzer
Copy link
Contributor Author

Thank you for looking into it again. Well spotted; no worries, I should have noticed that the functionality wasn't handling modules properly.

I've changed it to empty!(@memoize_cache(foo)) as you've asked. It is a macro call because the module of the caller has to be determined.

The new functionality is also documented again in the README. I've changed a to 2a to change

 julia> @memoize_cache(x)
  IdDict{Any,Any} with 1 entry:
    (1,) => 1

into

 julia> @memoize_cache(x)
  IdDict{Any,Any} with 1 entry:
    (1,) => 2

which I think is more clear to readers.

@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

I've changed it to empty!(@memoize_cache(foo)) as you've asked. It is a macro call because the module of the caller has to be determined.

I don't think that would work in another module. Try the example I gave:

module A
@memoize foo(x) = x
end

using A: foo
@clear_memoize_cache foo

If you implement memoize_cache(::typeof(foo)), we'll get the behaviour we want, which is that the cache is associated to the function foo, and not to the Symbol foo.

@rikhuijzer
Copy link
Contributor Author

Isn't that what is tested with

  using .MemoizeTest
  using .MemoizeTest: custom_dict

  empty!(@memoize_cache(custom_dict))
  @test custom_dict(1) == 1
  @test MemoizeTest.run == 3
  @test custom_dict(1) == 1
  @test MemoizeTest.run == 3

  empty!(@memoize_cache(MemoizeTest.custom_dict))
  @test custom_dict(1) == 1
  @test MemoizeTest.run == 4

in the tests? If I understand correctly, this is the example you gave

julia> using Memoize

julia> module A
       using Memoize
       @memoize foo(x) = x
       end
Main.A

julia> using .A: foo

julia> @memoize_cache(foo)
IdDict{Any,Any}()

julia> foo(3)
3

julia> @memoize_cache(foo)
IdDict{Any,Any} with 1 entry:
  (3,) => 3

julia> empty!(@memoize_cache(foo))
IdDict{Any,Any}()

@rikhuijzer
Copy link
Contributor Author

rikhuijzer commented Dec 11, 2020

Oh, and the build appears to be failing on Julia 1.1 with

ERROR: LoadError: "Main.MemoizeTest.custom_dict" is not defined in module Main

It seems that I need to fix that.

@rikhuijzer
Copy link
Contributor Author

On second thought, I'm not going to invest time on Julia 1.1. I did that at GiovineItalia/Gadfly.jl#1496 and it was not a good time investment.

Let me know if I can make changes to improve the functionality on Julia 1.5. If Julia 1.1 is a must, then feel free to close this PR.

Copy link
Collaborator

@cstjean cstjean left a comment

Choose a reason for hiding this comment

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

I wouldn't mind dropping 1.1 if it makes things easier and it's necessary.

src/Memoize.jl Outdated Show resolved Hide resolved
@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

in the tests? If I understand correctly, this is the example you gave

Right, sorry that I didn't read the code properly.

@rikhuijzer
Copy link
Contributor Author

Right, sorry that I didn't read the code properly.

No problem; it happens

I've now used parentmodule, which is great! 🚀 In Julia 1.1, I get

ERROR: LoadError: UndefVarError: ##Main.MemoizeTest.custom_dict_memoized_cache not defined

In Julia 1.2 the tests pass, so I've bumped the Julia version to 1.2.

@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

Thanks!

@cstjean
Copy link
Collaborator

cstjean commented Dec 11, 2020

Will merge when the checks are green again.

@cstjean cstjean merged commit 6a9dfd3 into JuliaCollections:master Dec 11, 2020
@cstjean cstjean mentioned this pull request Dec 15, 2020
@cstjean cstjean mentioned this pull request Dec 30, 2020
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.

2 participants