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

Fix a bug in the macro #25

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Fix a bug in the macro #25

merged 2 commits into from
Jul 11, 2024

Conversation

tjjarvinen
Copy link
Collaborator

Description of input that causes the bug

Using following definition causes the bug

AtomsCalculators.@generate_interface function AtomsCalculators.calculate(
       e::AtomsCalculators.Energy, 
       sys, 
       calc::MyType, 
       pr=nothing, 
       st=nothing; 
       kwargs...
 )
    nothing
end

This definition however works

AtomsCalculators.@generate_interface function AtomsCalculators.calculate(
       ::AtomsCalculators.Energy,   # note removal of e from this line
       sys, 
       calc::MyType, 
       pr=nothing, 
       st=nothing; 
       kwargs...
 )
    nothing
end

The cause of the bug

The bug can be inspected by calling

expr = Meta.parse("function AtomsCalculators.calculate(
       e::AtomsCalculators.Energy, 
       sys, 
       calc::MyType, 
       pr=nothing, 
       st=nothing; 
       kwargs...
 )
    nothing
end")

After which you get the bug by calling

expr.args[1].args[3].args[1].args[2].value  # Should return :Energy, but gives error

To get to the root of the error you can call

julia> expr.args[1].args[3].args
2-element Vector{Any}:
 :e
 :(AtomsCalculators.Energy)

Comparing that to the working case

working_expr = Meta.parse("function AtomsCalculators.calculate(
       ::AtomsCalculators.Energy, 
       sys, 
       calc::MyType, 
       pr=nothing, 
       st=nothing; 
       kwargs...
 )
    nothing
end")
julia> working_expr.args[1].args[3].args
1-element Vector{Any}:
 :(AtomsCalculators.Energy)

Solution

Based on the above, the type of calculation is always on the last element of expr.args[1].args[3].args.
Thus using following call solves the bug and works for all the cases

expr.args[1].args[3].args[end].args[end].value  # Should return :Energy or calculation type in general

I changed the index for the last argument as well. It should read AtomsCalculators.Energy, where AtomsCalculators is the first index and Energy or calculation type the last index (2nd in practice). This is just to be safe for the future.

I also added (altered) a test to verify that all use cases are now tested.

Severity of the bug

This bug has been in AtomsCalculators from the beginning and not been encountered before, so I don't expect this having been causing issues.

The only use case where this could be an issue and where it was discovered is, when you have a wrapper calculator and you want to have specific calls for it.

@tjjarvinen
Copy link
Collaborator Author

This is a pretty straight forward bug, so I will merge this once the tests work and tag a new version.

So, we get this resolved fast without spending peoples time on it. Feel free to comment though, if you have something to say.

@tjjarvinen tjjarvinen changed the title Fix bug with the macro Fix a bug with the macro Jul 11, 2024
@tjjarvinen tjjarvinen changed the title Fix a bug with the macro Fix a bug in the macro Jul 11, 2024
@tjjarvinen tjjarvinen merged commit cca635f into master Jul 11, 2024
4 checks passed
@tjjarvinen tjjarvinen deleted the macro_bug_fix branch July 11, 2024 23:39
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.

1 participant