remake dopedxpy-sc-fermi #46
Conversation
|
Looking good @alexsquires. I like the mix-in/abstract class approach. The Ofc the user could iterate over all possible vertex/limit combinations if they knew what they were doing, which would be a good stab at this and get you most of the way there I imagine (kind of like a 'band structure path' approach), but still is only covering certain 'high-symmetry' paths in chem pot space. So if a grid approach was also possible (where e.g. the user can just set the chempot spacing in eV, or total number of grid points), that would also be v nice I think – should be doable with some of the Sorry to be piling on the requests, but I guess the other main use case I'd imagine for this part of the code would be for the more complex defect thermodynamics analysis that Also just fyi, about your earlier |
|
Yeah, dw this is all on the list, should be ready to go by the end of the
week, I sort of trashed everything to start again (for about the 6th time
now), and just adding everything back in piecewise to make sure it's all
consistent with the latest thermodynamics object.
…On Thu, 15 Feb 2024 at 04:58, Seán Kavanagh ***@***.***> wrote:
Looking good @alexsquires <https://github.com/alexsquires>. I like the
mix-in/abstract class approach.
The _interpolate_chempots() function is quite nice, and I think will
likely be one of the mostly-used features from these additions. From what
we were discussing before in the #doped Slack, would it be possible to
also integrate a grid-scanning approach too? As in, the current function
linearly interpolates between two points (chem pot limits), which is nice
and v useful in many cases, but if for instance we have a 3D chemical
potential space, our min/max of the property of interest could also lie
somewhere in the middle of this space, rather than along any linear path
between two vertices/limits.
Ofc the user could iterate over all possible vertex/limit combinations if
they knew what they were doing, which would be a good stab at this and get
you most of the way there I imagine (kind of like a 'band structure path'
approach), but still is only covering certain 'high-symmetry' paths in chem
pot space. So if a grid approach was also possible (where e.g. the user can
just set the chempot spacing in eV, or total number of grid points), that
would also be v nice I think – should be doable with some of the scipy
grid space interpolation functions I think? And/or pymatgen chemical
potential diagram methods maybe?
If it was possible to implement relatively painlessly, could we add these
two options? ☝️ 🙏
Sorry to be piling on the requests, but I guess the other main use case
I'd imagine for this part of the code would be for the more complex defect
thermodynamics analysis that py-sc-fermi allows, where you want to fix
certain defect/species etc concentrations and perform some constrained
solutions. Is it possible to integrate this to the fermi_solver code?
Also just fyi, about your earlier _format_defect_name() question, yes it
is actually quite externally useful and I'll make it a public function in
the next (minor) release.
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMK74XGZEUFF3TPF7MZHH3YTXZ5JAVCNFSM6AAAAABBVDPFSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBVHE2TIMRVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@adair-nicolson , could you re-parse your Cu2SiSe3 data now that everything is relatively final and I can add the grid searching stuff back in? |
|
@kavanase proposing this as the full functionality - do you want to take a look at the example ipynb while I finish off the tests before I get to comfortable? |
# Conflicts: # examples/CdTe/CdTe_example_thermo.json
|
@alexsquires sorry for the delay in getting to this! Busy time... Looking very very nice, thanks very much for your work with this! 🙌 Some small queries from me:
py_fs = FermiSolverPyScFermi(defect_thermodynamics=thermodynamics, bulk_dos_vr=vasprun_path, multiplicity_scaling=32)
A general question: (will also look into the code in more detail myself)
|
|
Fyi, the |
|
@alexsquires just to flag, it seems like there are some unused files included here in the CdTe examples directory. Before merging can we check that only the ones needed are included, to avoid bloating the repo? |
…is is more intuitive?
…in `doped` concentration calcs, due to inefficient `defect.element_changes` in `pymatgen`; avoided now)
…functions, and fix bug in `generate_annealed_defect_system`
|
Hi @alexsquires!
|
|
todo:
|
# Conflicts: # docs/Tutorials.rst # doped/core.py # doped/thermodynamics.py
|
@alexsquires are you still working away on this or do you want me to look over? Just checking! |
|
Mostly there, just checks and warning suppression to go
…On Sat, 6 Apr 2024, 19:51 Seán Kavanagh, ***@***.***> wrote:
@alexsquires <https://github.com/alexsquires> are you still working away
on this or do you want me to look over? Just checking!
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMK74SFPG6DLBBD3HS5QRTY4A73PAVCNFSM6AAAAABBVDPFSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRGE3DMMBRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
# Conflicts: # examples/chemical_potentials_tutorial.ipynb
|
Important Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@kavanase has something changed with the chemical potentials? Cu2SiSe3 example no longer has limits wrt the elements, CdTe does? |
|
@alexsquires I'm not totally sure what you're referring to here. I checked the chempots json files in the examples folders ( CdTe: Cu2SiSe3: |
|
Also just to mention from looking through there, for consistency and reducing user confusion, it would be good to use the same e.g. from def get_equilibrium_fermi_level(
self,
bulk_dos_vr: Union[str, Vasprun, FermiDos],
chempots: Optional[dict] = None,
limit: Optional[str] = None,
el_refs: Optional[dict] = None,
temperature: float = 300,
return_concs: bool = False,
effective_dopant_concentration: Optional[float] = None,
) -> Union[float, tuple[float, float, float]]:
r"""
...
chempots (dict):
Dictionary of chemical potentials to use for calculating the defect
formation energies (and thus concentrations and Fermi level).
If ``None`` (default), will use ``self.chempots`` (= 0 for all chemical
potentials by default).
This can have the form of ``{"limits": [{'limit': [chempot_dict]}]}``
(the format generated by ``doped``\'s chemical potential parsing
functions (see tutorials)) and specific limits (chemical potential
limits) can then be chosen using ``limit``.
Alternatively this can be a dictionary of chemical potentials for a
single limit (limit), in the format: ``{element symbol: chemical potential}``.
If manually specifying chemical potentials this way, you can set the
``el_refs`` option with the DFT reference energies of the elemental phases,
in which case it is the formal chemical potentials (i.e. relative to the
elemental references) that should be given here, otherwise the absolute
(DFT) chemical potentials should be given.
limit (str):
The chemical potential limit for which to
determine the equilibrium Fermi level. Can be either:
- ``None``, if ``chempots`` corresponds to a single chemical potential
limit - otherwise will use the first chemical potential limit in the
``chempots`` dict.
- ``"X-rich"/"X-poor"`` where X is an element in the system, in which
case the most X-rich/poor limit will be used (e.g. "Li-rich").
- A key in the ``(self.)chempots["limits"]`` dictionary.
The latter two options can only be used if ``chempots`` is in the
``doped`` format (see chemical potentials tutorial).
(Default: None)
el_refs (dict):
Dictionary of elemental reference energies for the chemical potentials
in the format:
``{element symbol: reference energy}`` (to determine the formal chemical
potentials, when ``chempots`` has been manually specified as
``{element symbol: chemical potential}``). Unnecessary if ``chempots`` is
provided/present in format generated by ``doped`` (see tutorials).
(Default: None)
...
"""
fermi_dos = self._parse_fermi_dos(bulk_dos_vr)
chempots, el_refs = self._get_chempots(
chempots, el_refs
) # returns self.chempots/self.el_refs if chempots is None
... |
…np.isclose` over `assertAlmostEqual` etc), pull chempots from DefectThermodynamics if present and not supplied...
defect_levels argument when annealing becomes a boolean