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 several examples #23

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Conversation

mikibonacci
Copy link
Contributor

  • diamond
  • graphite
  • GaAs => bands are still a bit not good
  • Al

- diamond
- graphite
- GaAs => bands are still a bit not good
- Al
@eimrek eimrek self-requested a review May 14, 2024 15:11
Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

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

thanks @mikibonacci, the tool seems to be working well and the examples are great. Just in the convert-to-json.py file, the idea was that it would work without manually changing it for any of the example folders (just by putting the folder path as an argument). Therefore, the code should contain meta information about all the systems (e.g. the qpts to use). it also will act as a documentation on how the final json file was produced in each case and so that they could be easily recreated, if needed (e.g. if there are some updates/corrections to the files). If you can just adapt it accordingly, i'd be happy to approve/merge this.

@@ -15,22 +15,23 @@

system_name = os.path.basename(os.path.realpath(folder_name))

with open(os.path.join(folder_name, "scf.in")) as fhandle:
with open(os.path.join(folder_name, "GaAs.scf.in")) as fhandle:
Copy link
Member

Choose a reason for hiding this comment

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

I think here we should keep the convention that's defined above, i.e. forcing to use filenames of scf.in, scf.out, matdyn.modes. Would it make sense to rename the files as such? then in principle it's easy to regenerate the json files without having to manually change this code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I was not considering this issue, I'll change it back (and modifiy the filenames accordingly)

matdyn_modes = fhandle.read()

pretty_name_dict = {"BaTiO_3": "BaTiO<sub>3</sub>"}
pretty_name_dict = {"GaAs": "GaAs"}
Copy link
Member

Choose a reason for hiding this comment

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

I think this dict should contain entries for all the examples that have a subscript. So GaAs, is not needed, but BaTiO_3 should be kept and everything else that has subscripts should be added (although i'm not sure why it wasn't before). If the entry doesn't exist, i think the rest of the code will just use the correct system name automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it


highsym_qpts_default = [[0, "Γ"], [20, "M"], [40, "K"], [60, "Γ"]]
highsym_qpts_default = [[0, "L"], [40, "Γ"], [80, "K"], [100, "X"], [140, "Γ"]]
Copy link
Member

Choose a reason for hiding this comment

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

Here, this is the default qpts, that was the same for all the 2D materials. Before, the only expection was BaTiO3. I think this should be kept as it was, unless there is a better default.

highsym_qpts_dict = {
"BaTiO_3": [[0, "X"], [20, "Γ"], [40, "M"], [60, "Γ"], [100, "R"]],
"GaAs": [[0, "L"], [40, "Γ"], [80, "K"], [100, "X"], [140, "Γ"]],
Copy link
Member

Choose a reason for hiding this comment

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

Here you should put all the qpts that do not use the default qpts defined above. I think this is all the 3d examples that you added. BaTiO3 should also be kept here.

starting_reps_dict = {"BaTiO_3": (3, 3, 3)}
#supercell.
starting_reps_default = (3,3,3)
starting_reps_dict = {"GaAs": (3, 3, 3)}
Copy link
Member

Choose a reason for hiding this comment

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

same here. by default, use (5, 5, 1), but for 3d materials, use (3, 3, 3).

@mikibonacci
Copy link
Contributor Author

Hi @eimrek, thanks a lot for the review. I will fix what you suggested as soon as possible. Also, I am checking the correctness of the qpath for my new added examples.

fixed back the convert-to-json script, and added also some custom qpts and reps
dict for the new cases.
… points for U|K->G path,

to have a better interpolation (there is a crossing point just outside U|K).

Added specific path for FCC examples that I added: Diamond, Aluminum and GaAs.
@eimrek eimrek self-requested a review June 5, 2024 15:15
Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

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

One small thing to fix & the pre-commit. Will merge when this is done.

@@ -19,20 +19,29 @@
scf_input = fhandle.read()
with open(os.path.join(folder_name, "scf.out")) as fhandle:
scf_output = fhandle.read()
with open(os.path.join(folder_name, "matdyn.modes")) as fhandle:
with open(os.path.join(folder_name, "modes")) as fhandle:
Copy link
Member

Choose a reason for hiding this comment

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

But some files are still using "matdyn.modes" file, e.g. BaTiO_3

Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

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

thanks @mikibonacci, everything looks good now 👍

@eimrek eimrek merged commit 818df1e into materialscloud-org:master Jun 6, 2024
2 checks passed
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