-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add relative (no vacuum) alignment feature #1
Conversation
bapt/__init__.py
Outdated
ax.text(x + bar_width/2., ip + 1.65*pad, | ||
f"VBO: {compound['vbo'] - data[i-1]['vbo']:+.2f} eV", zorder=2, | ||
ha='center', va='top', size=label_size-3, color=name_colour, | ||
fontfamily='serif') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep this the default font to be consistent with the rest of the style of the plot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep no problem. Thought it looked slightly nicer with a different font at the time, but I see that a consistent style is better 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kavanase, thanks for this, I think this is a nice idea!
I've made some comments on the PR.
Basically, try and keep the plot style consistent:
- The same font should be used throughout
- Not more than 2 font sizes in the plot (ideally use 1 but I think a smaller font for the offset is ok).
- Lines should all be the same thickness.
Finally, can you add a yaml file example also?
Thanks again for this. Nice to see bapt
getting some love.
bapt/__init__.py
Outdated
ax.text(x + bar_width/2., ea - pad, | ||
f"CBO: {compound['cbo'] - data[i-1]['cbo']:+.2f} eV", zorder=2, | ||
ha='center', va='top', size=label_size-3, color=name_colour, | ||
fontfamily='serif') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
bapt/__init__.py
Outdated
ax.yaxis.set_major_locator(MultipleLocator(1)) | ||
for spine in ax.spines.values(): | ||
spine.set_zorder(5) | ||
ax.tick_params(which='major', width=1.75*_linewidth) # prettify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the width controlling the thickness of the line? In which case, can you keep the line thickness the same as all other axis lines for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was just to change the major axis tick linewidth to make it obvious that the zero of energy was set to the VBM of the first compound:
To give this:
Instead of this:
Either way, I suppose it's clear enough without changing the width, and looks a bit cleaner and more consistent, without changing it, so I'll change it now.
bapt/cli.py
Outdated
parser.add_argument('-i', '--ip', | ||
help='List of ionisation potentials (comma separated).') | ||
parser.add_argument('-e', '--ea', | ||
help='List of electron affinities (comma separated).') | ||
parser.add_argument('-cbo', '--cbo', default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abbreviated arguments should only be one letter long. I.e., if the long argument is --cbo
, the short one should be something like -c
. If -c
is already taken by another option, you can either come up with a different letter or leave it as a long argument only.
So this option, vbo and eg, should should be:
-c, --cbo
-v, --vbo
-e, --eg
bapt/cli.py
Outdated
parser.add_argument('-vbo', '--vbo', default=None, | ||
help='List of valence band offsets (comma separated). ' + | ||
'(Relative to first compound)(-> No vacuum alignment)') | ||
parser.add_argument('-eg', '--eg', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think --band-gap
is actually a better long argument. It is more clear what it means immediately. You can use -b
as an abbreviation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will do.
The yaml file should be along these lines: |
Also, can you go ahead and make the change in the readme to point to the new image url. |
One final thing: I added a contributors section to the readme! :) |
Ok, I've made all requested changes, and checked that it all still works as it should. I've added a |
Thanks again for this. Just to let you know, I made a few more edits, such as removing the "VBO:" and "CBO:" strings from the plot. I also, removed the reliance on f-strings. They are really nice but I don't think the adoption of python 3.6+ is high enough just yet. |
No worries, sounds good!
…________________________________
From: Alex Ganose <[email protected]>
Sent: Tuesday, January 14, 2020 11:08:17 PM
To: utf/bapt <[email protected]>
Cc: Kavanagh, Sean <[email protected]>; Mention <[email protected]>
Subject: Re: [utf/bapt] Add relative (no vacuum) alignment feature (#1)
This email from [email protected] originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders list<https://spam.ic.ac.uk/SpamConsole/Senders.aspx> to disable email stamping for this address.
Thanks again for this.
Just to let you know, I made a few more edits, such as removing the "VBO:" and "CBO:" strings from the plot.
I also, removed the reliance on f-strings. They are really nice but I don't think the adoption of python 3.6+ is high enough just yet.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1?email_source=notifications&email_token=AMIYBIPWU4XWPAF2VVNBKK3Q5ZAWDA5CNFSM4KFXIIYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI6ONUA#issuecomment-574416592>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMIYBILEDNKRPVFMNYG2PUDQ5ZAWDANCNFSM4KFXIIYA>.
|
Use the optional arguments
--eg
and--vbo
or--cbo
to specify relative alignments of bands. I've added an example toREADME.md
and help information.bapt
knows which plot to produce(vacuum alignment or not) based on whether
--eg
&--vbo
/--cbo
or--ip
&--ea
was specified."Vacuum level" changes to "conduction band", and the axis limits are set so that the highest CBM is 1 eV below the upper limit, and the lowest VBM is 1 eV above the lower limit.
Example:
bapt -n Cs\$_2\$AgBiBr\$_6\$,Cs\$_2\$AgSbBr\$_6\$,Cs\$_2\$AgSbCl\$_6\$ -eg 1.774,1.366,1.6 -cbo 0.247,-0.4 -o alignment.png --font-size 10 --show-axis
gives:Checking that the original plot function works as expected:
bapt --name "ZnO,MOF-5,HKUST-1,ZIF-8" --ip 7.7,7.3,6.0,6.4 --ea 4.4,2.7,5.1,1.9 -o normalalign.png
gives:Checking the errors work as expected:
Checking
bapt -h
works as expected:Note:
If merging, you might need to change the image source line in
README.md
from<img src="https://raw.githubusercontent.com/kavanase/bapt/master/examples/cli_novac.png" height="300">
to<img src="https://raw.githubusercontent.com/utf/bapt/master/examples/cli_novac.png" height="300">