-
Notifications
You must be signed in to change notification settings - Fork 131
Improve type safety and documentation in powerups modules #1332
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
Conversation
|
Hi @tomdemeyere , Thanks for working on this. Could you elaborate on how this new change affects usage of the powerups, and also check if the failing tests have anything to do with your changes? Thanks in advance! |
|
Thanks for the docstr updates @tomdemeyere! Some comments for stylistic consistency, |
|
@tomdemeyere can you look at why the cp2k tests are failing from these changes? |
|
Thanks for you comments @esoteric-ephemera I did not know about "or" I always thought NumPy style was with pipes, it's nice to know! Tests should be passing now. |
@JaGeo No runtime changes in this PR, only type hinting. The # Previous behavior
my_job: Job
my_job = update_user_kpoints_settings(my_job, ...)
# `my_job` is now Job | Flow | Maker type due to the return type of the function
#Current behavior
my_job: Job
my_job = update_user_kpoints_settings(my_job, ...)
# `my_job` is still the same exact type `Job` due to the TypeVar |
|
To me everything looks fine 😀. Are any comments still open from your side @esoteric-ephemera ? |
|
Thanks @tomdemeyere ! :) |
Summary
Made changes to
atomate2/vasp/powerups.pyandatomate2/common/powerups.pyJobTypeTypeVar for functions accepting union of types such asJob | Flow | Makerso that the return type by the static type checker is the same as the input type.Potential problem: at some point we can find
flow: Flow, additional_fields: dict, class_filter: Maker = BaseVaspMaker, the class filter then get send toupdate_custodian_handlerswhere we can findcode = class_filter.name.split(" ")[1]Here an instance property is called on what is supposed to be a type, I assume this is done because in practice the property is only implemented in the
Makerbase class and conventionally overridden by a class attribute in most subclasses (hacky).@JaGeo @naik-aakash
Checklist
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running
ruffandruff formaton your new code. This willautomatically reformat your code to PEP8 conventions and fix many linting issues.
Run ruff on your code.
type check your code.