search

siesta-project / aiida_siesta_plugin

Source code for the AiiDA-Siesta package (plugin and workflows). See wiki
Other
6 stars 11 forks source link

Iterator params help #76

Closed pfebrer closed 3 years ago

pfebrer commented 3 years ago

Hey Emanuele, I improved the display of the parameters in iterators so that it looks better for users, specially in a jupyter notebook, where it shows an accordion:

params_help

Hope you like it!

PS: This will also look good in monday's GM presentation :)

bosonie commented 3 years ago

Hi @pfebrer96, sorry for the delay. Please fix the pre-commit hooks.

pip install .[dev]
pre-commit run --all-files
yapf -i aiida_siesta/utils/iterate_absclass.py
pre-commit run --all-files

Read the pylint errors and fix them, they are self explanatory. For instance use ModuleNotFoundError, not a generic exception. Use understandable names for variables, not "e". In case of not fixable situation use the pylint disable.

I saw aiida-core depends on IPython, so all good, but, in general, we should be very careful when we add new calls to external packages. I will add a page on the wiki where we list all these calls.

While reviewing I noticed that maybe kpoints_density is not the right word to support. In fact, the density is defined from a distance. See here: https://aiida.readthedocs.io/projects/aiida-core/en/latest/reference/apidoc/aiida.orm.nodes.data.array.html#aiida.orm.nodes.data.array.KpointsData.set_kpoints_mesh_from_density I would change to kpoints_dinstance, but we keep also kpoints_density for back compatibility (I made a release last week). It would be nice to have it in the list with a "deprecated in favor of kpoints_dinstance" next to it. Let me know your thoughts!

pfebrer commented 3 years ago

I saw aiida-core depends on IPython, so all good, but, in general, we should be very careful when we add new calls to external packages. I will add a page on the wiki where we list all these calls.

IPython is not an extra dependency. I just use it in _ipython_display_, which is called only in an ipython environment (e.g. jupyter notebook). If you launched an ipython environment, you either have IPython or you can do magic :)

Use understandable names for variables, not "e".

I have to remove that, it was only for debugging.

I would change to kpoints_dinstance, but we keep also kpoints_density for back compatibility (I made a release last week). It would be nice to have it in the list with a "deprecated in favor of kpoints_dinstance" next to it.

kpoints_distance sounds defenitely better! I don't think we need to keep kpoints_density, probably no one has used it yet :sweat_smile:.

pfebrer commented 3 years ago

It should be good now. I haven't changed kpoints_distance, maybe you can do it in a separate commit/PR?

bosonie commented 3 years ago

Ok, we will sort it in another PR, you are right. Because we made a release, this means that people in the future installing aiida-siesta==1.1.0 will have to use kpoints_density. They will make workchains using this feature and if we remove it, their workchains will break after an update of aiida-siesta. I know, very remote chances, but I prefer to follow the indication of semantic versioning. I will do it, don't worry! Thanks for the work @pfebrer96! Still missing to fix a pre-commit request, but for this time I will lift you from this small burden and do it myself.

pfebrer commented 3 years ago

Still missing to fix a pre-commit request, but for this time I will lift you from this small burden and do it myself.

Yes, I saw that here in the PR. Running pre-commit run --all-files in my laptop passed all the tests though, where can the difference come from?

bosonie commented 3 years ago

Probably a different version of pre-commit or other packages, In our package, these are pinned to a particular version (see the setup.json). If you did pip install pre-commit, you got the newest version. I had to pin the versions long ago for some dependency problems, I have to look back to it and see if we can move on with new versions.