atoMEC's logo is missing on PyPI's project description. For a working example, see https://pypi.org/project/pandas/.

Update 09/11: In an attempt to fix this (#213), the logo is also now not showing on the GitHub docs page

Running the tests (#135) yields many warnings like:

We need to replace all np.matrix statements with ndarrays.

Currently the sqrt grid cannot be used with GGA functionals. At the very least a warning should be raised.

We use sphinx==4.0.2, but Sphinx latest version is 4.4.0. See https://www.sphinx-doc.org/en/master/changes.html

Check, why the step for publishing to PyPI got skipped for release v1.1.0. This step should be triggerd for a tagged commit. Even though a bumpversion minor ... creates a tagged commit locally, pushing it without the --tags flag will not trigger the pipeline and probably not even if the tag is subsequently pushed.

• Find a better criteria, maybe creating a release out of the tag on GitHub is a good choice

In the writeoutput module output is formatted in an outdated style. Since Python 3.6 f-string formatting is recommended. We should bring the code up-to-date.

Steps to perform:

1. Create a PR from develop to master #109
2. Merge the PR
3. Adjust date-released: ... in CITATION.cff (on master)
4. Create a tagged (and signed) commit on master with bumpversion minor --allow-dirty (check changes with git show or git diff HEAD^)
5. Check out develop and do a git merge master --ff
6. Push develop and master to origin
7. Create a new release out of the tag and add release notes: https://github.com/atomec-project/atoMEC/releases → Draft a new release
8. Check if release got published to PyPI

It seems some math is being rendered by Sphinx but most of it not. One of the issues may be that we need to somewhere import physics and amsmath packages with something like the following command in conf.py:

imgmath_latex_preamble = r"""                                                                                                                                                                                      
\usepackage{amsmath}                                                                                                                                                                                               
\usepackage{physics}                                                                                                                                                                                               
"""

At the moment, if math (TeX) expressions in the docs contain errors that lead to invalid TeX, those errors will become visible only when after the docs are built. It would be nice to allow the CI to catch such errors before the documentations goes online.

I have already done some research, but have not found a quick or easy solution.

See also: https://docs.mathjax.org/en/latest/options/input/tex.html#tex-formaterror

The Hartree potential integral is performed in two parts:

In the code this is done as:

Since we have a discrete grid we need to choose where the splitting point of the Hartree integral is (ie whether to use <= or < in the above). At the moment, it seems the split is not right when considering the integral discretization. So the inequalities need to be swapped (which makes a small difference to the numerics).

Currently, the boundary condition applied at the LHS grid-point forces the (transformed) orbitals to be exactly zero. This is not strictly correct, in particular when we have l=0 angular quantum number.

This doesn't yet seem to be having an adverse impact on the computation of observables, but it would be interesting to see the impact of applying the correct boundary conditions. In the current Numerov diagonalization scheme, this would involve appropriate changes to the hamiltonian matrix in the top LH corner.

See the extract from Eli's thesis for the correct constraints (of course, our grid is just a log grid, we have no dependence on atomic number Z.)

atoMEC uses the joblib library for a simple parallelization over spin and l quantum numbers when diagonalizing the Hamiltonian. It is recommended in joblib to dump large arrays to a file which is then deleted after.

Currently in atoMEC this writing to file does not work when multiple simulations are performed simultaneously from the same directory (i.e. quite likely on on HPC), because the data file for a given run is overwritten by the data file for another run.

Simple fix will be just to randomize the name of the data file.

The codecov reports are showing a lower percentage coverage than what is seen locally. This seems to have been triggered by renaming pressure_test.py to pressure_test_log.py, and introducing a new test pressure_test_sqrt.py in PR #179.

This page gives advice on finding the cause of the issue. In this case, I'm pretty sure we know the cause, as described above. I don't know how to fix it however.

Over time, some docstrings / comments have been neglected and do not reflect the code accurately. At some point, all docstrings should be checked for accuracy.

This is probably an ideal task for chatGPT (particularly the API).

python boundary_comp.py gives

func:'CalcEnergy' took: 83.0255 sec
Traceback (most recent call last):
  File "..../atoMEC_import/tests/boundary_comp.py", line 27, in <module>
    print("Total free energy with dirichlet :" + str(energy_dirichlet.F_tot))
AttributeError: 'dict' object has no attribute 'F_tot'

In setup.py the version is given as 0.1.0, whereas in CITATION.cff it is 1.0.0. Should we make these consistent? @DanielKotik I defer to your judgement on which is more appropriate.

(Also, I'm the only author listed in setup.py. I guess this ok since we can't list everyone there)

I have added a short description (taken directly from the README) and a few suggested topics. @acangi feel free to make any changes you would like.

A warning is currently issued if the temperature is outside of the range [0.01, 3.5] Hartree. A message is then printed that the "normal temperature range for atoMEC is 0.01 - 100 eV". There are two problems here:

1. The printed range does not actually match correctly to the range which triggers the warning
2. 100 eV is a bit of a random choice. We can certainly do temperatures that high, indeed we can often reach up to 1000 eV. It depends on the density.

I think we should essentially scrap these warnings. Somewhere in the documentation we should include a section on calculation issues in which we can mention the conditions under which atoMEC struggles, and possible strategies in those cases,

I noticed that the mirror from this repository to https://github.com/casus/atoMEC iseems not to be working. On the CASUS mirror it does not have the most recent updates to the code, with the last update being on 07.03.22 . Maybe it's because the PRs made since then weren't approved before being merged? But I don't think that's the reason because #117 was approved and the mirror didn't update. So maybe it was the one of the commits made on March 7th that caused this error.

Similar problem with properties as in PR #16.

The laplace function in mathtools module gives some numerical errors on the RHS boundary, due to the numpy.gradient function which uses finite differences.

This contaminates the kinetic energy density and thus the total kinetic energy with a small (but significant when computing pressures) numerical error. See for example the attached, we see the k.e.d. seems to approaching an asymptotic limit but the final 3 points are incorrect.

See https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/.

Steps required to create a new release as outlined in

• #110

should be documented somewhere, either in the Sphinx docs or in the Wiki for later reference.

Currrently, the only unbound electron treatment supported is "ideal". Since we perform a full diagonalization of the Hamiltonian and have access to a large number of eigenstates, we can use these eigenstates for the unbound electrons, like a "discretized continuum".

To make this work, the way that bound electrons are currently treated can basically be copied for unbound electrons, only now a new matrix such as "l_unbound" should be introduced which only takes into account the positive energy eigenvalues.

See run https://github.com/atomec-project/atoMEC/actions/runs/1676995381.The updated workflow file from PR #119 contains a syntax error, resulting in

Error : .github#L1
Invalid type for `on`

Some warnings appear in new test workflow:

• your are using a rather old version of the cache action (v1 instead of v3), an update should resolve some of the above issues
• change runs-on: ubuntu-18.04 to a more recent version

See this comment. Probably nicer ways of outputting data from calculations. Note that not only the <x>_to_csv functions have potential for improvement but a lot of the writeoutput module is a bit hacked together. We might also want to introduce options to store various objects (e.g. using pickle), maybe bundled up together, for re-use in postprocessing / restarting calculations.

Looks good.
I think the docstring in models.ISModel is not rendering correctly due to some incorrect formatting... But even if it were rendering there would the same error as we saw in PRs #18 and #16. I will fix later today.

Mhh, for me this docsting looks good. Also, there are now warning messages anymore.

Strange there are no warnings, but I don't see any of the class parameters displaying? And the propertys have no docs

Originally posted by @timcallow in #19 (comment)

atoMEC uses a sparse linear eigenvalue solver from scipy which has a tendency to break down when a large number of eigenfunctions are required, particularly for the "neumann" boundary condition. This is often the case e.g. at very low densities: the solver can only return the lowest magnitude eigenvalues, whereas we seek the actual lowest eigenvalues (i.e. negative ones with large magnitudes). This means we often have to ask for many more eigenvalues than actually needed.

Solution: Adding a constant to the KS potential shifts the eigenvalues by the same constant. If we choose a large enough number then all eigenvalues will have positive sign and we don't need to request so many arbitrary eigenvalues. Will see if this works.

The chemical potential (config.mu) is really a property of the staticKS.Orbitals class and thus should be moved there. Storing it as a variable in config causes a bug when post-processing a set of orbitals under different conditions, because the occupation numbers are recalculated with the last saved value of config.mu instead of their actual chemical potential.

Following PR #92

Job test-docstrings fails in gh-pages workflow. The checks performed are done via

pydocstyle --convention=numpy atoMEC

All the errors given, need to be fixed for the docs pipeline to run successfully.

Each push to the mirror repository triggers workslfows. This is not supercritical because deploying to GitHub pages and PyPI fails due to missing permissions of the mirror, but these and all other workflows do not have to run at all.

Currently the only treatment for unbound electrons available is "unbound", we want to add a "thomas_fermi" option.

See the notes attached for how the unbound density and energy should be computed via the Thomas-Fermi approach (the key equations are boxed).

notes_210215b (3).pdf

atoMEC currently relies on the config.py module to store various global variables. This worked ok for the initial purpose of the code, which was basically just set up to solve the KS-DFT equations for an average-atom model.

However, it's becoming clear to me that this a major problem when one wants to use the code in a modular way as more functionality is added, particularly post-processing tools for a given output of a KS calculation. Such issues are not limited to just the config.py module, some classes are initialized in such a way that it is not possible (without great difficulty) for a user to create their own instances, rather they only work if created via a KS-DFT calculation.

Fixing this issue will require a significant reformulation of the code... But needs to be done!

The defaults used by atoMEC were set in the initial development of the code. Since then, more advanced options have been added which significantly enhance the accuracy of the code. The defaults should be updated to use the "best" options (either most accurate or efficient, depending on the circumstance).

Black sets line lengths to 88 by default (PEP8 recommends however a line length of 79 characters). Either way, I have noticed that black does not reformat docstrings to adjust the line length. @timcallow can you confirm this? If we have to manually adjust the line length for docstrings, I would suggest to use flake8/pycodestyle to check against it (also in the pipeline).

We first have to agree on the number of characters per line (I am fine with both 79 and 88, but not more). If this differs from black's default, we should add a pyproject.toml with

[tool.black]
line-length = 79

Currently only the lda family of xc-functionals is supported in atoMEC. We should also add GGA functionals since they just need the gradient of the density as input.

When using multiple cores with joblib, sometimes the following error is raised:

Exception during reset or similar
Traceback (most recent call last):
  File "/home/callow46/.local/lib/python3.8/site-packages/sqlalchemy/pool/base.py", line 697, in _finalize_fairy
    fairy._reset(pool)
  File "/home/callow46/.local/lib/python3.8/site-packages/sqlalchemy/pool/base.py", line 893, in _reset
    pool._dialect.do_rollback(self)
  File "/home/callow46/.local/lib/python3.8/site-packages/sqlalchemy/engine/default.py", line 543, in do_rollback
    dbapi_connection.rollback()
sqlite3.ProgrammingError: SQLite objects created in a thread can only be used in that same thread. The object was created in thread id 139675633149760 and this is thread id 139675095406336.
Exception closing connection <sqlite3.Connection object at 0x7f08a8e3e030>

The code continues to run after the error is raised and it does not affect the results. However it's annoying since it doesn't look good (and I'm not sure but maybe is capable of causing something critical). It seems random as to whether this error appears or not.

It seems the sqlalchemy library is called indirectly through the mendeleev library imported into atoMEC. This only performs some initialization of the Atom object and joblib is supposed to work in a self-contained way, i.e. multiple threads should only be used during the diagonalization and then the code should return to single threading. So I'm not sure why joblib is interfering with sqalchemy. Either way it would be good to prevent this error from happening, or at the very least tell atoMEC just to ignore this error (I'm not sure how to do this since I don't know when exactly the error is induced).

@DanielKotik any ideas?

In the EnergyAlt class, the total energy is computed using an alternative method (sum over the eigenvalues and subtracting the integral over the Hxc potential, \int dr n(r) v_Hxc(r)).

If this approach is used and config.v_shift=True, then the constant shift in the eigenvalues must be compensated for by an equal and opposite term in the potential integral. Currently, this is not done.

This issue is related to GitHub caching badges. This issue can be solved as described here

For example, in Chromium open README.md and use the Web Developer Tools to inspect the badge in order to retrieve the respective https://camo.gitubusercontent... URL, see

Now clear the cache with curl -w "\n" -s -X PURGE https://camo.githubusercontent.com/..

We want this repository to be mirrored to the CASUS organisation.

Due to the dependence on the rather strange pylibxc library, atoMEC is not installable for Python > 3.9.

We need to add a warning in the README. But also test exactly which Python versions it is compatible with.

Running pydocstyle --convention=numpy <file.py> produces errors such as:

staticKS.py:523 in public method `E_xc`:
        D401: First line should be in imperative mood; try rephrasing (found 'dict')
staticKS.py:523 in public method `E_xc`:
        D403: First word of the first line should be properly capitalized ('Dict', not 'dict')

which clash (especially the first error) with how we document property tags (which follows the advice in https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html).

In workflow python-style-checks in step Check docstrings following message appears:

Run pydocstyle --convention=numpy atoMEC
WARNING: The /home/runner/work/atoMEC/atoMEC/pyproject.toml configuration file was ignored, because the `toml` package is not installed.

When a review is approved, the build-and-deploy-pages pipeline gets triggered but fails with

build-and-deploy-pages
Action failed with "not found deploy key or tokens"

The expected behaviour is, that this pipeline is skipped when a PR gets approved and only gets triggered, when the actual merge happens. Even better would be, if building the pages gets triggered when a PR is created and build+deploy finally on merge.