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

Remove matscipy dependency #14

Merged
merged 3 commits into from
Dec 11, 2019
Merged

Conversation

SebastianM-C
Copy link
Member

@SebastianM-C SebastianM-C commented Dec 9, 2019

The neighbourlist tests were moved to the MolSimPy package and that package was added as a conditional dependency with Requires.jl as discussed in #13

src/nlist.jl Outdated Show resolved Hide resolved
@cortner
Copy link
Member

cortner commented Dec 9, 2019

Is there a way to have an if-else version of @require so that a warning or error message can be printed when somebody tries to load the ASE neighbourlist?

@SebastianM-C
Copy link
Member Author

I'm not sure. I'll look into it.

@SebastianM-C
Copy link
Member Author

I'm not sure it's possible given JuliaPackaging/Requires.jl#41

@cortner
Copy link
Member

cortner commented Dec 9, 2019

Ok, then if I can indulge in your patience a little longer, could you try something the following:

function neighbourlist(at::ASEAtoms, cutoff::Float64)::MatSciPy.NeighbourList
   if ASE.hasmatscipy
      return MatSciPy.NeighbourList(at, cutoff)
   else 
      @warn("some warning / error message") 
      return nothing 
   end 
end

And the @require statement needs to set the flag. I think that may have to be done via Ref if I recall correctly.

@SebastianM-C
Copy link
Member Author

SebastianM-C commented Dec 10, 2019 via email

@SebastianM-C
Copy link
Member Author

I moved the neighbourlist function to ASE as you suggested and removed it from MolSimPy (to avoid conflicts when both are loaded); See also the last two commits here: https://github.com/JuliaMolSim/MolSimPy.jl/commits/master

I have tested this (ASE.jl) locally on windows and it works (i.e. test pass). If you would like to extend the CI to windows (or use the previously mentioned github actions workflows), let me know.

@SebastianM-C
Copy link
Member Author

I think what remains to be done is to add CI to MolSimPy.jl (see JuliaMolSim/MolSimPy.jl#1)

@cortner
Copy link
Member

cortner commented Dec 10, 2019

Many thanks - I’ll review and merge tomorrow!

@cortner cortner merged commit be2637a into JuliaMolSim:master Dec 11, 2019
@cortner
Copy link
Member

cortner commented Dec 11, 2019

Very elegant. THank you!

@cortner
Copy link
Member

cortner commented Dec 11, 2019

I ll register a new version later, but I’ll first want to test my other packages

@SebastianM-C SebastianM-C deleted the molsimpy branch December 12, 2019 15:35
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