lsmo-epfl/aiida-zeopp

AiiDA plugin for zeo++

aiida-zeopp's Issues

Expected file list for zeo++ option with Boolean flag

I recently realized that when we are using zeo++ flags like res where they accept Boolean flag, even if we set the flag to False, plugin still puts the expected output file in the list which results in crashing even though the calculations are done.
The example on the effect of this issue happened in my WorkChain when I wanted to modify the zeopp.parameters at some point in the workchain and realized the issue. Currently, I resolved it by deleting the key from the dictionary but thought to report here, in case, we decide to fix it.

update README for example usage

workflow untested with aiida-core 1.0

decide how to use `output_parameters` link

See here
Populating the output_parameters link allows to check results via calc.res.<TAB> shortcut

Let's use the plugin a bit and then decide how to proceed - whether to parse the default stdout [1] or whether to merge the dictionaries gathered from the individual parsed files.

[1] network HKUST-1.cif prints

Opening File: HKUST-1.cif
Starting Voronoi decomposition
Box dimensions:
  va=(26.343000 0 0)
  vb=(0.000000 26.343000 0)
  vc=(0.000000 0.000000 26.343000)

Total particles = 624

Internal grid size = (5 5 5)

Using voro++ with radii for particles.
Performing Voronoi decomposition.
Order 4 vertex memory scaled up to 16
Volume check:
  Total domain volume  = 18280.820976
  Total Voronoi volume = 18280.820976
Voronoi decomposition finished. Rerouting Voronoi network information.
Finished rerouting information.
Finished Voronoi decomposition

where can one find a complete API description?

The readme in zeo++0.3/documentation includes parameters like -sub and -fsub that are nowhere to be found on the examples of the homepage.
Where can I find an official & complete list of the supported command line parameters?

fix "create-code" utility

can currently end up creating new codes every time
see #33 (comment)

one possibility is that it gets confused if there are already 2 localhost-test around (in which case it cannot retrieve by label anymore)

allow allowAdjustCoordsAndCell flag

see lsmo-epfl/zeopp-lsmo#6

Force 1 cpu resources

Since Zeopp does not allow for parallelization,
force the resources setting to:

builder.metadata.options = {
    'resources': {
        'num_machines': 1,
        "tot_num_mpiprocs": 1,
    },
    'withmpi': False,
}

to avoid taking a full node if not correctly specified.
Shouldn't there be a setting in AiiDA that constrain serial codes' plugin to force this setting?

Raise warning if CIF file shows signs of zeo++ incompatibility

As (re)discovered by @mpougin, the zeo++ CIF parser is very fragile, and CIFs it does not understand can result in weird errors (even "out of memory" in her case).

If some of these issues are easy to detect, the prepare_for_submission function could check for signs of these and raise a warning or even an exception.

@mpougin @danieleongari @pzarabadip
If we could collect examples of "working" and "broken" CIF files here, we could then implement those in the plugin.

warn for unknown element symbols

Another point discovered with CIFs from the CSD: pymatgen spits out the symbol "D0+" when the structure contains deuterium, which is then not understood by zeo++

Unable to find radius for D0+ in table. Please provide it

zeo++ actually has a radius for deuterium, so it's not really its fault (more pymatgen's).

However, it might make sense to add a check in aiida-zeopp in prepare_for_submission:
if no radiifile is provided, one could try to parse the CIF file and check whether there are any element symbols unknown to zeo++ (see link above for the known ones).

In any case, this issue is mostly for reference - it's probably best to fix this at the pymatgen side

Consider upgrade monty version to >= 3.0.2

I can't resolve dependencies in aiidalab because of the monty package that is required to be:
https://github.com/ltalirz/aiida-zeopp/blob/30f8f6007ad00d4f6a5b7dd83191f1aa0d4d1b68/setup.json#L37

Would it be possible to migrate it to 3.0.2 or higher?

move from artifacts to caching

In principle, Github actions suggests to use caching for e.g. pre-compiling zeo++
https://help.github.com/en/actions/configuring-and-managing-workflows/caching-dependencies-to-speed-up-workflows#comparing-artifacts-and-dependency-caching

However, as @pzarabadip mentions, there are open issues with this approach
actions/cache#144

Let's monitor those and switch the CI to use caching once it works properly.

switch to new testing framework

testing via singularity has proven to be a dead end - it is simply not possible to reliably pull a singularity image on travis.

need to switch to an alternative way (e.g. build docker with zeo++ inside - or compile zeo++ on travis and cache the result).

zeo++ -sa output inconsistent

The output

 @ HKUST-1.sa Unitcell_volume: 18280.8   Density: 0.879097   ASA_A^2:
        3545.59 ASA_m^2/cm^3: 1939.51 ASA_m^2/g: 2206.26 NASA_A^2: 0
        NASA_m^2/cm^3: 0 NASA_m^2/g: 0 Number_of_channels: 1
        Channel_surface_area_A^2: 3545.59 Number_of_pockets: 0
        Pocket_surface_area_A^2:

is missing a value for the pocket surface area (should probably be zero)

Python3 only release?

Would it be possible to make a new release with Python3 support only? It will allow upgrading many dependencies. I am having problems with it when doing dependency resolution for aiidalab package.

surface_area_sa density has no units

verdi data parameter show 3911
{
  "ASA_A^2": 802.835, 
  "ASA_m^2/cm^3": 1251.47, 
  "ASA_m^2/g": 1672.62, 
  "Channel_surface_area_A^2": 802.835, 
  "Density": 0.748207, 
  "NASA_A^2": 270.403, 
  "NASA_m^2/cm^3": 421.506, 
  "NASA_m^2/g": 563.354, 
  "Number_of_channels": 1, 
  "Number_of_pockets": 1, 
  "Pocket_surface_area_A^2": 270.403, 
  "Unitcell_volume": 6415.16
}

Also

• this is a zeo++ bug (same in the .sa format)
• same for unitcell_volume
• strictly speaking, the quantities should be called ASA_A^2, ASA/V_m^2/cm^3, ASA/M_m^2/g as they are not the same (same for NASA)