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

Added yaehmop with band structure calc #339

Merged
merged 19 commits into from
May 25, 2021

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Jul 29, 2018

This is still a work in progress, but this commit adds
yaehmop with band structure calculations. It runs, but
several features (especially in the GUI) still need to
be coded.

Signed-off-by: Patrick Avery psavery@buffalo.edu

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@psavery
Copy link
Collaborator Author

psavery commented Jul 29, 2018

As you can see from the images below, I've managed to get the basic functionality of yaehmop and the yaehmop extension working in Avogadro2. I can generate a band structure similar to the one in Avogadro1.

Avogadro1 yaehmop band structure:
avogadro1-band-structure

Avogadro2 yaehmop band structure:
avogadro2-band-structure

The current Avogadro2 band structure dialog (it is very similar to the Avogadro1 band structure dialog):
image

One really nice feature of the VTK plot in Avogadro2 is that it will tell you the x and y values of the curves if you put your mouse over a spot on the curve.

There are still several things that need to be done before this PR is complete, though.

  • Marcus, do you want me to move the yaehmop download to the OpenChemistry repo? If that's the case, should yaehmop download when the user first runs cmake (assuming the correct cmake options are set)?
  • I need to make it so that spglib can put unit cells in their standard orientation
  • I need to add automatic k-point generation based on the detected space group
  • I need to make it so the vtk plot can use custom tick markers so we can use the special k points on the x axis
  • Several gui elements in the band structure dialog don't actually do anything right now (such as setting the fermi level). I need to make all the gui elements work.

Probably more stuff needs to be done, too...

Let me know if you have any thoughts or concerns about this, though.

@cryos
Copy link
Member

cryos commented Jul 29, 2018

Patrick, yes yaehmop should be made into an external project in the superbuild, avogadrolibs should depend on it, and the download should happen in the build step of that. At some point it would be good to make the option of download the binary or build it, but that can certainly be pushed off until later. We can talk about this tomorrow if that helps. The custom axis markers for the chart should be pretty easy, you can just pass the value numerically and what the label should be.

@ghutchis
Copy link
Member

Outside of this pull request, I should have some time before the UGM to code up parsing MO coefficients, etc. It's behind the force field work in priority, but I'd like to enable yaehmop for molecules.

@avo-bot
Copy link

avo-bot commented Aug 1, 2018

This pull request has been mentioned on Avogadro Discussion. There might be relevant details there:

https://discuss.avogadro.cc/t/august-development-update/2765/1

@psavery
Copy link
Collaborator Author

psavery commented Aug 4, 2018

I updated this branch so that it no longer downloads yaehmop, and I added a PR to the OpenChemistry repo to download yaehmop there instead.

This is still a work in progress, but this commit adds
yaehmop with band structure calculations. It runs, but
several features (especially in the GUI) still need to
be coded.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
This allows for both custom tick marker positions and custom
tick marker labels.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
@ghutchis
Copy link
Member

Is this still work in progress? Perhaps we can merge an initial version - seems like most things work?

@psavery
Copy link
Collaborator Author

psavery commented Aug 24, 2018

Unfortunately, it still is WIP. Several buttons in the pop-up dialog do not work. I should probably at least get them working (or remove them) before merging.

We should also merge the PR mentioned in my previous comment before this one.

Unfortunately, some unicode characters (such as Greek characters)
do not currently work. When we add a font to VTK that can display
unicode characters, we can start using the unicode gamma.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Data will, in the future, be save-able via a button on the plot.
This makes the 'display data?' buttons obsolete.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
This can save time if a plugin wishes to attempt to use a cached
space group rather than determine it algorithmically via spglib every
time it is needed.

This can also enable use to put space group information in the corner
render window of a crystal if it is desirable.

All of the information about a space group (the crystal system,
international number, schoenflies symbol, hall symbol, international
symbol, etc.) can be determined with just the hall number
(functions to do these are in avogadro/core/spacegroups.h).

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
This commit enables special k points to be generated automatically
using the space group of the crystal. It will first attempt to use
the cached space group, and if that does not exist, it will attempt
to use spglib to determine the space group.

It seems to work in all of the standard test cases that I tried.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
This enables the user to select the axis for the custom tick
labels.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
@psavery
Copy link
Collaborator Author

psavery commented Sep 11, 2018

This PR is almost complete. I just need to add the ability to plot and adjust the fermi level to the band structures.

It now also depends on PR #379.

I can squash some of these commits when complete if needed.

Signed-off-by: Patrick Avery <psavery@buffalo.edu>
Signed-off-by: Patrick Avery <psavery@buffalo.edu>
@@ -583,6 +592,9 @@ class AVOGADROCORE_EXPORT Molecule
UnitCell* m_unitCell;
Array<Residue> m_residues;

// This will be stored from the last space group operation
unsigned short m_hallNumber = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cryos
What do you think about initializing class members at their declaration like this?

Personally, I think it is a nice addition in C++11, since it allows for a default value for all constructors that can be overridden (if desired) by a constructor's member initializer list.

@psavery
Copy link
Collaborator Author

psavery commented Sep 23, 2018

By the way, this PR is pretty much complete and ready for review. The only thing I am waiting for is PR #379, since that is a dependency for this PR.

@ghutchis ghutchis added the enhancement feature changes / API changes label May 1, 2021
@github-actions
Copy link
Contributor

Here are the build results
macOS.dmg
Ubuntu1804.tar.gz
Ubuntu2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis ghutchis changed the title WIP: Added yaehmop with band structure calc Added yaehmop with band structure calc May 25, 2021
@ghutchis
Copy link
Member

I'm going to merge this. I'd love a better way to check the whole PATH, but that's a separate issue...

@ghutchis ghutchis merged commit 1925ee1 into OpenChemistry:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature changes / API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants