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

Implementation of custom element assignment from file #281

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

badarsh2
Copy link
Collaborator

This is one of the ideas that I had proposed earlier to account for the non-availability of element information in many of the trajectory formats. Particularly in situations where the number of atoms in the system is huge, it becomes impractical to use the CustomElementDialog tool. Hence, in addition to the custom element mapping tool in the build menu, I've included an additional option to import the element information from file. This creates a temporary molecule class instance for the file imported, and the element information is copied from this molecule to the molecule in question. For example, if a GROMACS trr trajectory has been read, one can use this option to import a .gro file (just like normal file import, which is done using Avogadro's readers or OpenBabel if available) and the details of the elements will be reflected in the trajectory molecule. (This does not explicitly create a CustomElementMap, I have just used the setAtomicNumber() function to achieve the aforementioned).

@badarsh2
Copy link
Collaborator Author

image

Copy link
Member

@ghutchis ghutchis left a comment

Choose a reason for hiding this comment

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

Generally looks okay to me...

}

QStringList CustomElements::menuPath(QAction*) const
{
return QStringList() << tr("&Build");
return QStringList() << tr("&Build") << tr("Assign &Custom Elements...");
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure on naming here. Generally I hear "topology" more - I'm not sure if one is more prevalent. What do you think?

m_progressDialog->setWindowTitle(tr("Reading File"));
m_progressDialog->setLabelText(
tr("Opening file '%1'\nwith '%2'").arg(fileName).arg(ident));
/// @todo Add API to abort file ops
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you just connect the cancel signal with m_fileReadThread => quit() instead?

.arg(m_molecule->atomCount())
.arg(mol->atomCount()));
} else {
size_t n = m_molecule->atomCount();
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 get a file, do you want to map bonds as well? For the molecular-based trajectories (even XYZ) this seems like a good idea.

@badarsh2 badarsh2 changed the title (WIP) Implementation of custom element assignment from file Implementation of custom element assignment from file Aug 2, 2018
@badarsh2
Copy link
Collaborator Author

badarsh2 commented Aug 2, 2018

Made the above suggested changes.

Copy link
Member

@cryos cryos left a comment

Choose a reason for hiding this comment

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

This can't be merged, it takes the background file format from the app, and adds it to a plugin. This causes duplicate symbol error on linking the application, and if we want to move this from the application to the libraries it would be better form to put it in the appropriate library and use the same implementation from there.

@badarsh2
Copy link
Collaborator Author

badarsh2 commented Sep 6, 2018

I moved the background file format script and headers to a new library folder and tried using these headers in the app, but mainwindow.cpp doesn't seem to recognise the header path, so I guess I'm missing out on something. Anything I should ensure while moving the files to a separate plugin and using them in the app?

@cryos
Copy link
Member

cryos commented Sep 6, 2018

They should be moved to qtgui, and would need the export macros adding as other classes do so that the symbols are exported. They would also need putting into the same namespace as the other classes, they don't need to be in an entirely new library.

@badarsh2
Copy link
Collaborator Author

@cryos I've made the suggested changes, and the plugin now uses the relocated backgroundfileformat class.

@ghutchis
Copy link
Member

@cryos - can you re-review this so we can merge if it's ready?

@ghutchis
Copy link
Member

Ping @cryos

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.

3 participants