Code formating, typing, clenup, docstrings ...

[marian-code]

I enjoy working with deepmd kit and I like that new functionality can be added quite easily as much of it is written in python. But the codebase has grown considerably and is now really hard to read and understand. This slows the developement and creates a high barrier for anyone that wants to implement new features to the code or even just check on somethink in the code. I think deepmd-kit could greatly benefit from use of some standardized formating, type hints and at least minimal standardized docstrings.

The changes I propose are:

• adhere strictly to some styleguide which will be enforced by chosen python lintner and also request that all PRs adhere to the chosen style guide. The benefits will be:
  • The code formating will be the same everywhere, so one does not have to deal with different code formating form different authors
  • Easily resolvable pull requests - everyone uses the same forrmating style so code review is easier
  • Many people have come across at least one of the common style guides like PEP8, Google, Black ... so naturally the code will be easier for them to understand from the begining
• Use type hints everywhere possible enforced by mypy static typechecker.
  • This also greatly improves code readability.
  • Static typecheckers can uncover aditional bugs in the code.
  • Much easier to navigate code and understand method parameters even without docstrings
• Use unified docstring style and require that any new additions to the code are properlly documented.
  • Again great enhancement to code readability.
  • Use of some common and popular style like numpy or google dostrings which most people are familiar with will make it easier to understand for newcomers

I have started the discussion here as I would be willing to implement the features outlined one by one but I need to know if such PRs would be accepted as it is quite a lot of work.

I suggest use of Black to enforce consistent style as it is really strict and does not give user the option to bring in his onw formating. Python typing standard library module for extensive types and MyPy static typechecker to enforce typehints. Lastly Numpy docstring standard as it is perhaps the most commonly used one.

These are just my suggestions, all is up for discussion. I would like to hear your thoughts on the matter.

[amcadmus]

Hi @marian-code

Thank you very much for the proposal. I totally agree with you that the code style and the documentation of DeePMD-kit need to be improved.

At the current stage, the interfaces of classes are subject to change. For example, the initializer of DescrptSeA takes a dictionary as input, which is definitely not a good definition, because a user does not know which key should be provided to the dictionary by only checking the interface.

I would suggest adopt the changes you are proposing after the change of interfaces.

Best regards,
Han

[marian-code]

Thank you for the quick reply @amcadmus,

I agree that in a way it makes sense to wait until the chances are finalized. But on the other hand as it is a lot of work and surely not everything will be changed I think it makes sense to start early. Improving code style and documentation can also help to identify places in code that need to be rewritten and may result in easier transition process.

Let me use the example DescrptSeA initializer you mentioned for clarification:

from typing import List
from typing_extensions import TypedDict

JDATA = TypedDict("JDATA", {
    'sel': List[int],
    'rcut': float,
    'rcut_smth': float,
    'neuron': List[int],
    'axis_neuron': int,
    'resnet_dt': bool,
    'trainable': bool,
    'seed': int,
    'type_one_side': bool,
    'exclude_types': list,
    'set_davg_zero': bool,
    'activation_function': str,
    'precision': str
})

class DescrptSeA ():
    def __init__ (self, jdata: JDATA):
        ...

Typing can be used to to clarify which keys with corresponding value types must be present. The JDATA type makes the interface a lot clearer without any rewrites. This may not be the absolute best solution to this exact problem but I think it ilustrates the point well enough.

If I may ask is there an estimated time when the API will be stable?

Maybe this could be done by opening a parallel branch to devel where the changes would be slowly implemented (at least on parts of the code that are considered stable enough) and then desired commits could be merged into devel branch using something like git cherry-pick perhaps?

In summary what I ment to say was that the work done even now when API is not stable is certainly not completely in vain. Even though that some things will need to be done more times over as the code changes I think there might still be a considerable benefit to this approach.

Best regards,

Marián

[amcadmus]

Hi Marián,

Thanks a lot for your suggestions.

You are referred to the new branch https://github.com/amcadmus/deepmd-kit/tree/api for the progress of the APIs. By far I have finished descrpt_se_a and fitting. Any comment and criticism is welcome!

We are planning to finish it as soon as possible, for example 1 to 2 weeks.

Best,
Han

[marian-code]

Thank you for the ref. to the api branch. I will have a look at it as soon as I find the time.

I didn't think you would be planing to finish in such short time. This brings new perspective. Now I agree it certainly makes sense to wait until the rewrite is complete and only than implement these changes.