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

[*.py] Standardise docstring usage of "Default to" #921

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

SamuelMarks
Copy link
Contributor

It's me again! - PS: I can split this into multiple PRs if you prefer.


I am trying to treat JAX, TensorFlow, PyTorch and Keras [amongst others] as data.

For example, translating the API hierarchy from Python to SQL; or generating type-safe help-text included CLIs. This will enable a number of new use-cases.

Unfortunately as it stands, your codebase lacks the type specificity for these use-cases. This is the first, probably of many, PRs to make your codebase consistent enough to be useful for these cases.

Additionally it'll generate better documentation for your primary use-cases; and make it clearer what types are being used where.

E.g., Defaults to 1. is ambiguous. Is 1 a float or an int?

Knowing the difference is then useful for continuous variable optimisation (e.g., Ray Tune or Google Vizier hyperparameter optimisation across the int or float domain). (as an aside; I am interesting in constraining the type numerical range more specifically also; like ASN.1 or Fortran allows)

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you! Question: how do we prevent future drift? Do you have a docstring linter we could run on CI?

keras_core/backend/jax/distribution_lib.py Outdated Show resolved Hide resolved
@SamuelMarks
Copy link
Contributor Author

@fchollet You're welcome.

As for a linter, I'd have to investigate the options and maybe extend them with defaults to linting:

Will see if I can send you a PR for it soon.

I think in the meantime having this consistency throughout the codebase gives new contributors a hint as to the expectation.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for investigating. I just assumed that you must have used some automation to create the PR, so perhaps this could be the basis for a very basic linter.

LGTM -- merging the PR now!

@fchollet fchollet merged commit 1f7d869 into keras-team:main Sep 20, 2023
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants