-
Notifications
You must be signed in to change notification settings - Fork 187
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
Refactor the Orbital code and fix bugs #2737
Refactor the Orbital code and fix bugs #2737
Conversation
Marked as [WIP] because I'd like @DanielMarchand to test if now this works (you can check out the branch in my fork) and if I didn't change the API. Also, @sphuber: I apparently get some weird errors from prospector in my tests (at least In my previous runs, like cyclic imports etc.) - is this related to the caching of pip or to something else? |
Coverage decreased (-5.4%) to 65.057% when pulling de8ccf0f3ea08f82f540441aefd67102c90ff886 on giovannipizzi:fix_2419_realhydrogen_orbitals into 899d803 on aiidateam:develop. |
1 similar comment
Coverage decreased (-5.4%) to 65.057% when pulling de8ccf0f3ea08f82f540441aefd67102c90ff886 on giovannipizzi:fix_2419_realhydrogen_orbitals into 899d803 on aiidateam:develop. |
I have just tested and the import works now! Thanks!! |
de8ccf0
to
274d6f0
Compare
@sphuber I fixed your comments and I also added a couple more tests - I think this is ready to be merged (if tests pass) |
@giovannipizzi the linter is known to be a bit buggy when it comes to cyclic imports. It is a bit weird that touching other files seems to trigger it. You could just try to add the disable statement in one of the files that it highlights as problematic. The only thing I can see quickly is that |
The thing that is actually needed is |
👍 Seems fair enough |
I added a commit that should solve the cyclic import problem - to avoid other analogous problems, the right approach seems to be to define the constants in a submodule |
7391dd3
to
383d19a
Compare
py3.6 linter is pickier... and the problem got worse! |
The intended aim of this commit is to avoid breaking backward compatibility while refactoring the existing code to model atomic orbitals and add tests.
383d19a
to
b50d061
Compare
The function is general enough to justify it being defined in `aiida.common` alongside with the `LinkType` enum. In addition, this prevents cyclic imports problems between the `aiida.engine` and `aiida.orm` modules.
b50d061
to
85442e8
Compare
@giovannipizzi I ended up fixing the problem in a different way, no more cyclic imports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @giovannipizzi
Thanks a lot @DanielMarchand |
The code was not tested (and now we add tests)
Moreover, the code required quite some cleanup and
refactoring.
Now the code should be cleaner (even if probably still
not perfect). The intended aim of this commit is to avoid
breaking backward-compatibility.
This fixes #2419