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

"default None" in docstring parameter descriptions considered harmful #1574

Closed
kandersolar opened this issue Oct 18, 2022 · 3 comments · Fixed by #1828
Closed

"default None" in docstring parameter descriptions considered harmful #1574

kandersolar opened this issue Oct 18, 2022 · 3 comments · Fixed by #1828

Comments

@kandersolar
Copy link
Member

A common python idiom for situations where a function's parameter can be left unspecified is to set its default value to None. In this usage None is understood not as a valid value for that parameter but rather as an indication to the function's body that it should treat the parameter as having taken no value. We could have chosen any python value for this task; we use None because it is convenient and idiomatic. One can view this pattern as a workaround to python's requirement that all parameters be specified either explicitly by the caller or implicitly by its default value. Take Javascript as a point of comparison, where unspecified parameters are automatically set to undefined without the function signature having to specify that default value.

As such, I don't think there is a good reason to say default None in docstring parameter descriptions in these situations; None is more of an implementation detail than a meaningful default value that the reader would be interested in. For example, consider get_total_irradiance's airmass parameter:

airmass : None or numeric, default None

Yes it's true that this parameter's default value is None, but I don't think that information is useful to the reader. Either airmass is required and they need to pass a numeric, or it isn't required and they don't need to touch that parameter. There is no situation where the user needs to say airmass=None in their own code. I think it would be better to use this style:

     airmass : numeric, optional

The useful information is still there, but without the distraction of None. It also renders fine in sphinx docs.

default None is fairly widespread in pvlib: $ git grep -n -i -F "default None" | wc -l returns 81. I propose we not use default None in docstrings in the future (except of course in the rare case when None is a legitimate value) and eventually edit existing docstrings to say optional instead.

P.S. for anyone who doesn't get the title of this issue: https://en.wikipedia.org/wiki/Considered_harmful

@mikofski
Copy link
Member

+1 for replacing docstrings with “default None” to “optional”

@adriesse
Copy link
Member

+2 for adding a description of what the default behavior actually is!

@wholmgren
Copy link
Member

Noting that pandas follows the "optional" pattern. I think pandas previously to stated "default None" but I didn't care to dig into it. Optional also follows the python typing pattern.

echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Nov 25, 2022
echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Feb 15, 2023
echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Feb 19, 2023
echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Jun 28, 2023
(?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$
$1$2 : $3, optional

(?<spaces> {8}| {4})(?<name>\w*) : None or (?<type>.*), optional$
$1$2 : $3, optional

(?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$
$1$2 : $3, optional

Over files: pvlib/**/*.py
echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Aug 4, 2023
(?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$
$1$2 : $3, optional

(?<spaces> {8}| {4})(?<name>\w*) : None or (?<type>.*), optional$
$1$2 : $3, optional

(?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$
$1$2 : $3, optional

Over files: pvlib/**/*.py
echedey-ls added a commit to echedey-ls/pvlib-python that referenced this issue Aug 6, 2023
1st-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$
2nd-F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn}one or (?<type>.*), optional$
3rd-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$

R: $1$2 : $3, optional
kandersolar pushed a commit that referenced this issue Dec 12, 2023
* Align reference

* Update irradiance.py

Exclude .*=| from negative lookahead

^(\s{8}|\s{4})(?!try|else|doi|DOI|Warning|Access|Requests|Note)(\w*): (?!.*:)(.+)$

Reason: just this line 🌝
Remaining matches are type annotations in modelchain.py and pvsystem.py

* Remove parenthesis in types

F: (?<spaces>\s{8}|\s{4})(?<name>\w*)\s?: (?<type>.*) \(optional, default=(?<default>.*)\)$
R:

* Remove none in type definition #1574

1st-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$
2nd-F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn}one or (?<type>.*), optional$
3rd-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$

R: $1$2 : $3, optional

* Change if none ocurrences (I)

F: If None,
R: If not specified,

* Change if none ocurrences (II)

F: If None
R: If not specified,

* Too long line

* Too long line

* fix doi external links

Regex find: (?:doi|DOI):(?!`)\s?(.*?)(\.\n|\n)
Replace: :doi:`$1`$2

* Apply cwhanse suggestion

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Apply cwhanse suggestion

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Apply cwhanse suggestion

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Apply cwhanse suggestion

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Modified cwhanse suggestion

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* Update modelchain.py

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* Some None's missed

F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn]one[ ,]*(?<type>.*), optional$
R: $1$2 : $3, optional
Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* ``none``'s, some more ``'s to variables and some other nones

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* Special ``none`` cases (please review)

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* Linter

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

* Run more regexes

F1: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*), default: None
F2: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<types>.*), default None$
F3: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one or (?<type>.*), optional$     zero results
F4: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*) or [nN]one, optional$     zero results

R: $1$2 : $3, optional

* More Nones

F: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*), default:? None\.?
R: $1$2 : $3, optional

F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one, (?<type>.*), default:? (?<def>.*)\.?
R: $1$2 : $3, default $4

F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one or (?<types>.*)
R: $1$2 : $3

F: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<types>.*) or [Nn]one(?<trailing>.*)
R: $1$2 : $3$4

F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one(?:,|or) (?<types>.*)
R: $1$2 : $3

* Update irradiance.py

* Revert "Special ``none`` cases (please review)"

This reverts commit b8f942b.

* Apply suggestions from code review

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Update irradiance.py

* Update pvlib/irradiance.py

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* Update irradiance.py

---------

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>
mikofski added a commit to mikofski/pvlib-python that referenced this issue Aug 1, 2024
use "optional" vs. "default None" per pvlib#1574

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
AdamRJensen added a commit that referenced this issue Aug 16, 2024
* coerce and rotate pvgis TMY data to desired tz and year

- add private function `_coerce_and_rotate_pvgis()`
- add `utc_offset` and `coerce_year` params to docstring for `get_pvgis_tmy`
- call private function if `utc_offset` is not zero

* test get_pvgis_tmy_coerce_year

check if utc_offset and coerce_year work as expected

* fix flake8 in test_pvgis_coerce_year

- remove whitespace
- shorter lines

* remove iloc for index in test pvgis coerce

- incorrect syntax for indices

* deal with leap year in pvgis when coercing

- if february is a leap year, when shifting tz, causes issues
- so replace february year with non-leap year

* fix space around operator in coerce pvgis

- also fix use ts for timestamp when fixing feb leap year

* fix pd.Timestamp in pvgis coerce year

- lower case "s" not TimeStamp

* Update v0.11.1 what's new for coerce pvgis tmy

- add description and links to issue/pr

* replace year and tzinfo in pvgis_tmy coerce year

- also use np.roll
- also make new index and dataframe instead of altering original
- removes need to sanitize original index February for leap year
- remove calendar import but add numpy and pytz
- code much simpler shorter, easier to read

* remove unused imports from pvgis.py for flake8

* change private function name to _coerce_and_roll_pvgis_tmy

* spot check rolled pvgis TMY values after converting tz

- fix Turin is actually CET which is UTC+1
- be DRY so use variables for test year and tz constants, versus WET and hardcoded
- check tz unchanged if default zero utc_offset
- use _ output args instead of indexing data[0]
- add comments

* Update utc_offset description

- explain setting utc_offset will roll data to start at Jan 1 midnight

* change coerce_year and utc_offset defaults to None in pvgis TMY

- update arg docstrings
- allow user to coerce year even if utc_offset is None or zero
- use 1990 as default if utc_offset is not None or zero, but coerce_year was unspecified
- add warning comment to be explicit and test identity to avoid unexpected implicit booleaness

* rename roll_utc_offset in get_pvgis_tmy

- refactor utc_offset everywhere including comments and docstring
- add additional test to coerce year even if utc offset is zero or none
- change tzname to 'UTC' (versus Etc/GMT or Etc/GMT+0) if replacing with zero utc offset

* Update pvlib/iotools/pvgis.py with suggestions

use "optional" vs. "default None" per #1574

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

* Update docs/sphinx/source/whatsnew/v0.11.1.rst

rename argument "roll_utc_offset" in whatsnew

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

* rename _coerce_and_roll_tmy, remove 'pvgis'

* rename index with new tz in coerce pvgis tmy

* allow tz of None in _coerce_and_roll_tmy

- treat tz=None as UTC
- allows get_pvgis_tmy to be simpler
- remove unnecessary comments

* clarify input tmy_data is UTC...

- ... in docstring of private function pvgis._coerce_and_roll_tmy()
- rename tmy_data
- name new_index explicitly using pd.DatetimeIndex()

* fix flake8 in _coerce_and_roll_tmy

---------

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants