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

Merging #9

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

Merging #9

wants to merge 80 commits into from

Conversation

arsalan-akhtar
Copy link

@arsalan-akhtar arsalan-akhtar commented Nov 23, 2020

Dear Nick
Hi i hope i done everything right!... plz just don't merge it yet I'm still doing changes ... i just open this pull for u to check things are in correct path

Cheers

@zerothi
Copy link
Member

zerothi commented Apr 8, 2021

How are things going? ;)

I really would like to get back to this. I'll probably have to post-pone until after the transiesta workshop, but could you please provide some more details. And later, it would be nice if pull requests were a bit smaller and more divided (then it is easier to review them ;))

Thanks again!!!

Copy link
Member

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

Lets discuss this Wednesday.

It really would help to separate things a bit. The different bits gets bloated and it is not easy for me to see where stuff belongs.

Also, I have to think about the documentation, since lua does not intrinsically support sphinx, other tools are currently more favoured, so I have to consider heavily what to do here...

from ase import Atoms
from ase.neb import NEB
import os, shutil, sys
from linecache import getline
Copy link
Member

Choose a reason for hiding this comment

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

not used, delete

parser = argparse.ArgumentParser()
parser.add_argument('-d', "--directory", type=str, default=".",
help='Directory where the fdf files are located, default is the current directory')
parser.add_argument('-n', "--nimages", type=int, default=5,
Copy link
Member

Choose a reason for hiding this comment

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

I would just use -n and --images it should be clear that it refers to number of.

help='Directory where the fdf files are located, default is the current directory')
parser.add_argument('-n', "--nimages", type=int, default=5,
help='Number of images that must be generated between the two states, default is 5')
parser.add_argument('-if', "--initialfile", type=str, default="initial",
Copy link
Member

Choose a reason for hiding this comment

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

change to -I and --initial

help='Number of images that must be generated between the two states, default is 5')
parser.add_argument('-if', "--initialfile", type=str, default="initial",
help='Name of the initial structure file (without extension), default is "initial"')
parser.add_argument('-ff', "--finalfile", type=str, default="final",
Copy link
Member

Choose a reason for hiding this comment

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

change to -F and --final

help='Name of the initial structure file (without extension), default is "initial"')
parser.add_argument('-ff', "--finalfile", type=str, default="final",
help='Name of the final structure file (without extension), default is "final"')
parser.add_argument('-m', "--method", type=str, default="li",
Copy link
Member

Choose a reason for hiding this comment

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

spell it out, also, probably use choices=('linear') to make it clear what they mean li can mean anything (almost) ;)


end

function DNEB:file_exists(name)--name
Copy link
Member

Choose a reason for hiding this comment

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

Also, some of these should be needed? No, if you use inheritance all should be good?

@@ -547,4 +547,27 @@ function NEB:info()

end


function NEB:file_exists(name)--name
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be a class method here. This looks more like a utility?

end
return NEB_F
end
--- Query the current force (same as `NEB:force` but with IO included)
Copy link
Member

Choose a reason for hiding this comment

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

Space to separate methods etc. Also looks like indentation problems?

@@ -0,0 +1,1821 @@
O ca nrl nc
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should mix these things here... This is better suited for Siesta.

@@ -0,0 +1,124 @@
--[[
Copy link
Member

Choose a reason for hiding this comment

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

You are mixing many things at once. It is easier for me to see what belongs where if we could have separated these things.
1 for tutorials, 1 for NEB stuff and 1 for documentation

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