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

Dl poly dev #1726

Draft
wants to merge 24 commits into
base: develop
Choose a base branch
from
Draft

Conversation

jounaidr
Copy link

No description provided.

@jounaidr jounaidr mentioned this pull request Nov 23, 2023
@trisyoungs trisyoungs added this to the Release 1.5 milestone Jan 12, 2024
Copy link
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

This is good - you have the guts of the thing in place. Seeing it written out like this and the way you have leveraged the FileAndFormat for this purpose makes me reconsider how we should be passing data around for these more complex file formats. Specifically, passing the long list of variables to the export function is functional, but not too pretty. It's beyond the scope of this PR, but creating intermediate classes to store the data in (e.g. DLPolyFieldFile) may be a better approach long term. This would make the DL_POLY stuff more like the implementation of Crystallographic Information File (CIF) handling, moving it beyond a simple I/O "option".

Copy link
Member

Choose a reason for hiding this comment

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

If you need to hack the CMakeLists.txt to build on your local machine then make sure not to commit those changes since this will break the CI in various places. I'm not sure exactly why you had to comment these bits out since they are disabled by default, however?

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate that you added this so you could compile DL_POLY within nix, but again this is outside of the Dissolve scope and shouldn't be included in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical file order within suffix sections.

#include "base/sysFunc.h"
#include "classes/configuration.h"
#include "data/atomicMasses.h"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

formats_ = EnumOptions<DlPolyFieldExportFileFormat::DlPolyFieldExportFormat>(
"FieldExportFileFormat", {{DlPolyFieldExportFormat::DLPOLY, "dlpoly", "DL_POLY FIELD File"}});
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

if (!parser.writeLineF("finish\n"))
return false;

int vdw = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int vdw = 0;
auto vdw = 0;

Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical file ordering.

@trisyoungs trisyoungs removed this from the Release 1.5 milestone Apr 8, 2024
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