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

cli invert axis, add get site all orbitals dos plot #149

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

naik-aakash
Copy link
Collaborator

Closes #142

Hi @JaGeo ,

DOS plotting options have been added

  1. --invertaxis option can plot DOS now via cli in chemistry conventions
  2. Using --orbital all in conjuction with site index for plotting dos, generates a plot with all available orbitals
  3. Tests have been updated

@naik-aakash
Copy link
Collaborator Author

Ready for review

@JaGeo
Copy link
Owner

JaGeo commented Sep 14, 2023

Thank you! I have the issue that it always plots the total dos as well. Is this on purpose?

@JaGeo JaGeo self-requested a review September 14, 2023 17:16
@naik-aakash
Copy link
Collaborator Author

Thank you! I have the issue that it always plots the total dos as well. Is this on purpose?

I intentionally kept that. But I can remove this hard coding. I thought it would be nice to always see the total dos when using other options for comparisons

@JaGeo
Copy link
Owner

JaGeo commented Sep 14, 2023

I don't think it's good. I barely could see the orbital-resolved DOS for some of the test cases.

@JaGeo
Copy link
Owner

JaGeo commented Sep 14, 2023

Can we make it optional? Or switch it off?

@naik-aakash
Copy link
Collaborator Author

Can we make it optional? Or switch it off?

Sure, we can make it optional . Will make changes tomorrow and let you know when I finish it.

@JaGeo JaGeo added the enhancement New feature or request label Sep 14, 2023
@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo, now the dos plotter should not add total dos by default and only adds it when -- addtotaldos arg is supplied along with dos-plot. However, now the dos-plot always needs to be used with some of the available args like -- elementdos, -- elementspddos. etc. This will also be made clear in the documentation. 😃

@JaGeo JaGeo merged commit 6311b4d into JaGeo:main Sep 15, 2023
@JaGeo
Copy link
Owner

JaGeo commented Sep 15, 2023

I think it is --spddos but this sounds fine as well. It's merged.

@naik-aakash naik-aakash deleted the dosplotter_new_features branch June 20, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DosPlotter - new feature request
2 participants