Allow for more general properties to be saved by the generic calculator

[gelzinyte]

In the main branch, only the properties defined in per_atom_properties or per_config_properties are saved by the generic calculator, even if an extra property (e.g., "polarization" for aims) is among the calculator's implemented_properties and in the calculator.results dictionary. The change tries to save all of the properties requested from the calculator and the missing or not implemented properties are caught by calc.get_property.

[bernstei]

I'm happy with it, although in principle this is probably better handled using https://gitlab.com/ase/ase/-/blob/master/ase/outputs.py?ref_type=heads, as ase.io.extxyz does now. Unfortunately, looks like the names you used, which are presumably from specific calculators you want to use, don't match the standard names there (e.g. fermi vs. fermi_level).

[gelzinyte]

Let me have a look at outputs.py. And I really am only interested in polarization, for now, so let's skip fermi (from vasp) and charges (from castep). Do you know why the quantum espresso tests are failing? They run ok for me locally, also when qe is installed with apt-get.

[bernstei]

There are some issues related to property free_energy not being implemented, which could have something to do with your changes. But many of the QE failures are because the executable gets a SIGABRT, which is weird, but it's not giving any more info as to why. Let me try to investigate.

[bernstei]

Is it just #352?

[bernstei]

I think yes - this is what happens when I run pw.x in the CI

Warning: card &CELL ignored
Warning: card / ignored
Warning: card &FCP ignored
Warning: card / ignored
Warning: card &RISM ignored
Warning: card / ignored

     Current dimensions of program PWSCF are:
     Max number of different atomic species (ntypx) = 10
     Max number of k-points (npk) =  40000
     Max angular momentum in pseudopotentials (lmaxx) =  3
*** buffer overflow detected ***: terminated

Program received signal SIGABRT: Process abort signal.

Backtrace for this error:
#0  0x7f10a4423e59 in ???
#1  0x7f10a4422e75 in ???
#2  0x7f10a404532f in ???
#3  0x7f10a409eb2c in pthread_kill
#4  0x7f10a404527d in gsignal
#5  0x7f10a40288fe in abort
#6  0x7f10a40297b5 in ???
#7  0x7f10a4136c18 in __fortify_fail
#8  0x7f10a41365d3 in __chk_fail
#9  0x7f10a4137db4 in __snprintf_chk
#10  0x5557fea0896e in ???
#11  0x5557fe8261d7 in ???
#12  0x5557fe88a007 in ???
#13  0x5557fe70a0c7 in ???
#14  0x5557fe708f22 in ???
#15  0x7f10a402a1c9 in ???
#16  0x7f10a402a28a in __libc_start_main
#17  0x5557fe708f54 in ???
#18  0xffffffffffffffff in ???
Aborted (core dumped)

[bernstei]

working on a fix to pw.x in #358. Probably have to merge from that one once it's working.

[bernstei]

@gelzinyte I just merged #358 into main, so if you merge from main now it should fix the QE test failures and we can see what real failures remain

[bernstei]

I think this looks better. Let's see how the tests go.

[bernstei]

Looks like these failures are probably real side effects of changes you're making.

[gelzinyte]

In the failing unit test no properties are given within wfl's md, so save_calc_results defaults to properties = list(atoms.calc.results.keys()), and all properties that were calculated and are in ase.calculators.calculator.all_properties are now saved by default. free_energy is calculated by default by EMT, and was among per_config_properties in the main branch. But in main, free_energy is never saved, because there isn't a at.get_free_energy() that could be called. Here, at.get_property() is called on all asked-for properties, and the test unexpectedly gets an extra property. (free_energy isn't saved in the preceding test_save_results, because properties are given explicitly.)

I think this change is small in practice and maybe even preferable? If you agree, I'd like to update the unit tests to also expect free_energy.

It's not a big change in how wfl functions will behave, because the generic calculator defaults to properties=["energy"," forces", "stress"]. md, minimahopping, neb and optimize would now end up with saving all the calculated properties that are also in all_properties. But that only includes free_energy, dielectric_tensor, born_effective_charges and polarization, only if computed by default and only if they are also among the calculator's implemented_properties.

[gelzinyte]

one of the docs examples also fails, let me have a look.

[bernstei]

I think this change is small in practice and maybe even preferable? If you agree, I'd like to update the unit tests to also expect free_energy.

I'm happy with updating the test to expect free_energy.

[gelzinyte]

The examples' test was failing because the xtb calculator calculates free_energy by default, but it's not among its implemented_properties, so calc.get_property("free_energy") fails. I have added an extra check for saved properties to avoid this. This should be it, once the tests pass.