Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding function annotations and type-hints. #53

Closed
wants to merge 0 commits into from

Conversation

alchemistcai
Copy link
Contributor

@alchemistcai alchemistcai commented Mar 23, 2023

  • Added(for IDEs' type checking and code auto-completion support):
    • Most function annotations and some variable annotations.
    • Some Type variables and classes.
    • *.pyi stub files for .c implementations of scoring functions and other math functions.
  • Changed:
    • class DockingModel is a generic type now.The objects attribute of DockgingModel[DFIREObject]'s subclass is always a list of DFIREObject.For type checking and code auto-completion too.
    • Doc-strings are moved underwards variables to make IDEs show doc-strings correctly.
    • Defualt parameters of [] are alterd to be None,less confusing.
    • Use generator expressions instead of list comprehensions when possible.
  • Fixed:
    • Some outdated __init__ calls of the subclasses of DockingModel because of the new membrane position parameter.
    • Fixed Spacepoints.copy() to Spacepoints.clone() in other classes' clone methods.
  • Known flaws:
    • Changes above are not tested actually.They are just checked statically by IDEs.My bios updated and I can't enter into linux now :(
    • __getitem__ and __setitem__s' index parameters are writed to be int,most of them accept slice and return lists of elements though.
    • NDArray,SpacePoints,Coordinates,List[float] and simliar types are not compatible in most function signatures' type checking.I'm looking for a more compatible interface for them.

@brianjimenez brianjimenez self-assigned this Mar 24, 2023
@brianjimenez brianjimenez added the enhancement New feature or request label Mar 24, 2023
@brianjimenez
Copy link
Member

Thanks for your contribution @alchemistcai . A recurrent error is killing all tests:

lightdock.test.bin.post.test_calculate_reference_points.TestCalculateReferencePoints.test_calculate_4IZ7_B ... Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.8.16/x64/bin/lgd_calculate_reference_points.py", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/runner/work/lightdock/lightdock/bin/lgd_calculate_reference_points.py", line 7, in <module>
    from lightdock.util.typing import List, NDArrayFloat1D, StrOrPath, Sequence, Union
  File "/home/runner/work/lightdock/lightdock/lightdock/util/typing.py", line 8, in <module>
    from lightdock.structure.residue import R
  File "/home/runner/work/lightdock/lightdock/lightdock/structure/residue.py", line 3, in <module>
    from lightdock.util.typing import Optional, ResID, Union, List, TypeVar
ImportError: cannot import name 'ResID' from partially initialized module 'lightdock.util.typing' (most likely due to a circular import) (/home/runner/work/lightdock/lightdock/lightdock/util/typing.py)

I don't use type hints on my daily basis, but I do see the advantage for other developers. So after fixing the tests and new bugs, I'd happy to merge this into main.

Since this PR requires a lot of testing and double-checking, I keep this in the fridge until I have some extra time.

@alchemistcai
Copy link
Contributor Author

Just entered into linux successfully again.I will fix them and test in linux recently.

@alchemistcai
Copy link
Contributor Author

All tests passed now.Phew!

@brianjimenez
Copy link
Member

Thanks @alchemistcai for the heavy work adding type hints to the current code. After a general overview of the changes, this is my opinion:

  • Type hinting is nice in general, but
  • Wrapping types in double quotes as strings is not

I think it would be better to use from __future__ import annotations in the code to avoid quotes on types and make the code also backward compatible for <3.8. Is that possible?

Maybe it is a good time for dropping support for 3.6 and 3.7 since they are or almost deprecated (https://endoflife.date/python). Moreover, nose has been also deprecated and it would be a good time to update to pytest or similar, and then wrap all changes in a new version, maybe it is also time for version 1.0!

Let me know your thoughts, specially on the quotes-hints concern. Cheers!

@alchemistcai
Copy link
Contributor Author

I hate quates for type subscriptions too.I tried to from __future__ import annotations in lightdock.utils.typing module but python 3.8 still can't ignore them at runtime.Maybe I didn't import it correctly.Should I add the statement in all python files?

@alchemistcai
Copy link
Contributor Author

I use python 3.10 and 3.11 more for type annotations and not sure how to make codes compatible and elegant in python 3.8 at the same time .

@brianjimenez
Copy link
Member

Before changing all the files, let me further investigate this and I will come back to you. As I mentioned in the previous comment, if we could avoid using quotes on 3.8, then it is good for me to just drop support for 3.6 and 3.7 and move on. Replacing nose is going to be dramatic too, but baby steps :)
PS: I don't have a lot of time to check this in the next days, but this doesn't mean that I won't do it. Sorry in advance for the delay!

@brianjimenez
Copy link
Member

Getting back to this PR after some time, sorry for the delay @alchemistcai

After analyzing pros and cons of type-hints, I come to the conclusion that they add more burden and have real limitations (for example NumPy associated structures).

I'd like to ask if it would be possible for you to create a new PR with all changes from this one, but without the type-hints associated code. There are some important fixes from this PR that should be added to the main branch, including *.pyi stubs. Once merged into next release branch, I'd work on replacing nose and updating the CI/CD pipeline.

@alchemistcai
Copy link
Contributor Author

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants