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

re-order add_feature arguments #633

Open
DaniBodor opened this issue Jul 22, 2024 · 3 comments
Open

re-order add_feature arguments #633

DaniBodor opened this issue Jul 22, 2024 · 3 comments
Labels
covariates modules from the 'features' subpackage disc Discussion needed, to better define the task/s stale issue not touched from too much time

Comments

@DaniBodor
Copy link
Collaborator

The function is currently defined by default as follows:

def add_features(
    pdb_path: str,  # noqa: ARG001
    graph: Graph,
    single_amino_acid_variant: SingleResidueVariant | None = None,  # noqa: ARG001
) -> None:

There is only 1 feature for which the pdb_path is actually used. However as currently implemented it must always be given. For this reason, it would make more sense if pdb_path is requested last, so that it can just be omitted in most add_features calls.

But I think it would be even better to always take **kwargs as an argument in this function. Any specific argument needed for the feature in question can be explicitly named, and all others can be "hidden" in the kwargs. This also opens up the possibility for adding other keywords to future (or user defined) features that are not currently used, while maintaining a default structure.

@DaniBodor DaniBodor added disc Discussion needed, to better define the task/s covariates modules from the 'features' subpackage labels Jul 22, 2024
@DaniBodor
Copy link
Collaborator Author

I wonder whether this would be considered a breaking change (requiring a new major version). On the one hand, calling the add_features function is done behind the scenes by the API. However, for any user defined features, this could now break the code.

@gcroci2
Copy link
Collaborator

gcroci2 commented Jul 29, 2024

Good points, thanks for raising the issue!

I wonder whether this would be considered a breaking change (requiring a new major version). On the one hand, calling the add_features function is done behind the scenes by the API. However, for any user defined features, this could now break the code.

Indeed, this will need a major release.

Copy link

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale issue not touched from too much time label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
covariates modules from the 'features' subpackage disc Discussion needed, to better define the task/s stale issue not touched from too much time
Development

No branches or pull requests

2 participants