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

Scope macro interactions by module #63

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

Conversation

jakobjpeters
Copy link
Contributor

Implements the API described here. Hopefully you'll hear back about how DynamicQuantities.jl does it soon :)

Turing.@model and MacroTools.@capture are still getting special treatment and I don't think they should be built-in to the macro table, but there's not currently a way to make the default behavior extensible. This PR improves the situation overall though!

Closes #43

Copy link

github-actions bot commented Aug 5, 2024

Benchmark Results

main c6b787d... main/c6b787dc5d25e4...
_stable/mode=disable 0.731 ± 0.019 μs 0.721 ± 0.019 μs 1.01
_stable/mode=error 0.526 ± 0.017 ms 0.682 ± 0.017 ms 0.772
_stable/mode=warn 0.519 ± 0.036 ms 0.676 ± 0.03 ms 0.769
time_to_load 0.0681 ± 0.00067 s 0.0694 ± 0.0018 s 0.981

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@MilesCranmer
Copy link
Owner

Can we leave register_macro! for backwards compatibility for 0.4 and have it declare the macro for Main (which was the previous default)?

@MilesCranmer
Copy link
Owner

MilesCranmer commented Aug 5, 2024

Does this check for hierarchies? Like if I define one behavior for Main, one behavior for Main.A, and one behavior for Main.A.B, will it select the most specific option?

(Maybe we should be relying on multiple dispatch to do this automatically for us...)

@@ -17,50 +17,53 @@ end
# Macros we dont want to propagate
const MACRO_BEHAVIOR = (;
table=Dict([
Symbol("@stable") => IncompatibleMacro, # <self>
(Main => Symbol("@stable")) => IncompatibleMacro, # <self>
Copy link
Owner

Choose a reason for hiding this comment

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

Just out of curiosity why have (::Module => ::Symbol) => ::MacroBehavior as the table? Does it hash faster or something compared to a tuple? Also why not have ::Symbol => [(::Module => ::MacroBehavior), (::Module => ::MacroBehavior)]?

Copy link
Contributor Author

@jakobjpeters jakobjpeters Aug 6, 2024

Choose a reason for hiding this comment

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

Also why not have ::Symbol => [(::Module => ::MacroBehavior), (::Module => ::MacroBehavior)]?

Purely because I didn't want to search the array and the module-macro pairs are unique anyways. I'm not otherwise attached to that implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it hash faster or something compared to a tuple

Oh, it's probably the same, I just like pair syntax and again am not attached to it.

@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented Aug 6, 2024

Can we leave register_macro! for backwards compatibility for 0.4 and have it declare the macro for Main (which was the previous default)?

Done!

Does this check for hierarchies? Like if I define one behavior for Main, one behavior for Main.A, and one behavior for Main.A.B, will it select the most specific option?

It didn't, but does now!

(Maybe we should be relying on multiple dispatch to do this automatically for us...)

How so?

@jakobjpeters
Copy link
Contributor Author

I speculated that mutating the table in the new test had a run-time order dependent effect, that seems to not be the case. I'm not sure what's causing the error.

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.

Cannot register macro with same name
2 participants