Add docstrings for modules/classes/methods

[amdecker]

Problem

The code lacked significant portions of documentation which reduces code readability and long-term maintainability. Pylint picked up the places lacking documentation.

Relevant pylint codes:
C0114 missing-module-docstring
C0115 missing-class-docstring
C0116 missing-function-docstring

Proposed solution

I used the following prompt for duck.ai (GPT-4o)
Please provide really concise and appropriate docstrings (module, class, and method) for the following code without changing any logic. Docstrings should follow Numpy standards and then I went though and revised them all.

Needs review:

• I'm not confident about the descriptions of some of the physics quantities that it filled in so they should be reviewed by someone with more physics knowledge. Here's an example, but there's more in the code:

            - 'ssimag' : array
                Inner boundary magnetic flux from the EFIT data.
            - 'ssibry' : array
                Outer boundary magnetic flux from the EFIT data.
            - 'psin' : array
                Normalized poloidal flux values.

Additional notes:

• I changed the name SharedInstanceFactory --> SharedInstance because I think it more closely follows a singleton design pattern rather than a factory design pattern.
• Removed _get_n_equal_1_amplitude because it has no functionality at the moment and we will revise the separation of fetching and calculating later
• Removed module_file_path_f in conftest.py because it is literally the same code as test_file_path_f

[EDIT by GLT: I added pylint ignore comments for the few issues here below.]
Obstacles to removing all instances of these Pylint error codes:

• https://github.com/MIT-PSFC/disruption-py/blob/dev/disruption_py/core/utils/math.py has multiple locally defined classes e.g. several are simply named Channel and I wasn't sure what to put as a docstring for them. I could write "Data Channel" but that does not help improve readability
• I did not change anything in multiprocessing.py under the assumption it will soon be removed

Test with

make pylint-only CODE=C0114,C0115,C0116

[gtrevisan]

awesome! it will take some time to review though... 👀

[gtrevisan]

I've added a commit to reformat all files to fit my style preference, that is, triple quotes always alone on a new line, and empty line after module docstring:

• D204 @ https://docs.astral.sh/ruff/rules/one-blank-line-after-class/
• D209 @ https://docs.astral.sh/ruff/rules/new-line-after-last-paragraph/
• D213 @ https://docs.astral.sh/ruff/rules/multi-line-summary-second-line/

I don't have strong feelings regarding the other rules.
also, I'm confused now by the various conventions (eg google vs numpy vs pep257).

anyway, automatically fixed with:

# poetry add ruff # -- will add it to the project in the future
poetry run ruff check --ignore ALL --select D204,D209,D213 --fix

[gtrevisan]

moved get_n1_bradial_parameters into the DIII-D drafts folder.

[gtrevisan]

LGTM, from a very very very rough review.

is there a ruff code to enforce a newline after the docstring in a function/method, similar to D204 (which enforces it after a class)?

EDIT: that is, the opposite of D202?
EDIT: also, is there the opposite of D200?

[amdecker]

Is it accurate to say the time from li, efittime = params.mds_conn.get_data_with_dims( r"\efit_aeqdsk:li", tree_name="_efit_tree", astype="float64" ) is a measurement time?

[amdecker]

@yumouwei @gtrevisan

[gtrevisan]

not really, I'd say it's the "time at which the internal inductance was computed using EFIT".

[yumouwei]

All EFIT parameters share the same time array so it shouldn't be "computed specifically for li". I'd just say "The EFIT time array/base (for the given shot)"

[amdecker]

Is this correct?

[gtrevisan]

smoothed signal of the measured plasma current.

[gtrevisan]

LGTM, @amdecker let us know if you had other physics-based doubts, please, then we can merge!
thank you for this tedious but solid PR.

and many thanks to @yumouwei for reviewing the physics methods, once again! 💪

[yumouwei]

I went through all of the changes and didn't notice any explicit problem, so I'd say we can merge this.

[gtrevisan]

great catch!