search

simphony / simphony-lammps

The LAMMPS engine-wrapper for the SimPhoNy framework
http://www.simphony-project.eu/
BSD 2-Clause "Simplified" License
8 stars 0 forks source link

Problem: lammps and ligghts engines are mixed in this wrapper #118

Closed mehdisadeghi closed 8 years ago

mehdisadeghi commented 8 years ago

Solution: remove liggghts support and simplify the whole package. There is a new wrapper for liggghts called simphony-liggghts.

@TobiasRasp @ahashibon Please see the changes. In the next step I'll simplify the repo name to simphony-lammps.

codecov-io commented 8 years ago

Current coverage is 90.95% (diff: 93.61%)

No coverage report found for master at 7d3f098.

Powered by Codecov. Last update 7d3f098...abb6fbc

nathanfranklin commented 8 years ago

@mehdisadeghi , @mehdisadeghi , @TobiasRasp there is a lot of overlap between lammps and ligghts especially when writing/reading the lammps/ligghts data file and the lammps/ligghts scripts. If you just have two repositories , you might end up with lots of duplicated code... maybe, you would want a third repository just containing the overlap.

Also note that there are some things in #106 branch that you might want consider using (or at least reviewing) regardless of what you end up doing.

In that branch there are some general bug fixes (e.g. 2a89fc35ba9c1fe596949b59916e0d11feab9a88 , b34dd485d7154ae92e63bd456fb0ea7d4d505ceb, 4be6016d57c0a91a2b30668c9d056f795aea4346) and lots of LIGGGHTS related stuff (although using an outdated material concept). There are the improvements for atom styles and atom style fixes (like https://github.com/simphony/simphony-lammps/blob/add_material_materialrelation/simlammps/config/atom_type_fixes.py#L15-L25) so that depending on atom style (currently granular or atomic are supported) simphony-lammps knows how script files and data files should be written and read. Although an example outdated Material class is used, I think this branch could just be updated to support the new definitions.

mehdisadeghi commented 8 years ago

@nathanfranklin It totally makes sense to put duplicated code into a third repository and reuse it in liggghts and lammps and any similar one. Our main purpose was to simplify the repository and reduce the complexity by moving each wrapper to a new repository (perhaps we could refactor the lammps repository instead and have two separated wrappers, each in its own package).

As you mentioned there are many fixes in branch 106. I'll definitely pick them up. I'll updated your code with new material definition.

Thanks for your thorough voluntary comment! :)