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

Add docstring inheriter #1258

Merged
merged 39 commits into from
Oct 8, 2024
Merged

Add docstring inheriter #1258

merged 39 commits into from
Oct 8, 2024

Conversation

YigitElma
Copy link
Collaborator

@YigitElma YigitElma commented Sep 21, 2024

Most of the documentation of our objectives has the same parameters that are inherited from the main _Objective class. This PR removes the repeated docstring from each objective and updates the docstring by inheriting which reduces the lines of code and also facilitates the maintenance. For example, when we add jac_chunk_size in #1052, we have to copy-paste the docs to every single objective which is tedious.

Introduces collect_docs function that creates docstring for common parameters and with option to overwrite user can give a custom definition for a parameter without changing the order of the docs

Resolves #879

Copy link

codecov bot commented Sep 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.47%. Comparing base (6c599b4) to head (5d3afd0).
Report is 40 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   95.45%   95.47%   +0.02%     
==========================================
  Files          96       96              
  Lines       23806    23896      +90     
==========================================
+ Hits        22724    22815      +91     
+ Misses       1082     1081       -1     
Files with missing lines Coverage Δ
desc/objectives/_bootstrap.py 97.18% <100.00%> (+0.04%) ⬆️
desc/objectives/_coils.py 99.19% <100.00%> (+0.01%) ⬆️
desc/objectives/_equilibrium.py 95.08% <100.00%> (+0.12%) ⬆️
desc/objectives/_free_boundary.py 97.07% <100.00%> (+0.03%) ⬆️
desc/objectives/_generic.py 97.60% <100.00%> (+0.07%) ⬆️
desc/objectives/_geometry.py 97.00% <100.00%> (+0.06%) ⬆️
desc/objectives/_omnigenity.py 96.36% <100.00%> (+0.06%) ⬆️
desc/objectives/_power_balance.py 89.79% <100.00%> (+0.21%) ⬆️
desc/objectives/_profiles.py 97.45% <100.00%> (+0.08%) ⬆️
desc/objectives/_stability.py 98.51% <100.00%> (+0.03%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes

@f0uriest
Copy link
Member

As part of this would also be good to make most of the "generic" arguments keyword only. Like

def __init__(self, eq, specific_arg1, *, target=None, bounds=None, other_args_shared_by_all=default, ...):

That way we can add more and reorder them in the future without breaking things

@f0uriest
Copy link
Member

Here's a sketch of what I was talking about at the meeting:

target_doc = """
    target : {float, ndarray}, optional
        Target value(s) of the objective. Only used if bounds is None.
        Must be broadcastable to Objective.dim_f.
"""
bounds_doc = """
    bounds : tuple of {float, ndarray}, optional
        Lower and upper bounds on the objective. Overrides target.
        Both bounds must be broadcastable to to Objective.dim_f
"""
weight_doc = """
    weight : {float, ndarray}, optional
        Weighting to apply to the Objective, relative to other Objectives.
        Must be broadcastable to to Objective.dim_f
"""

callable_target_doc = """
    target : {float, ndarray, callable}, optional
        Target value(s) of the objective. Only used if bounds is None.
        Must be broadcastable to Objective.dim_f. If a callable, should take a
        single argument `rho` and return the desired value of the profile at those
        locations. Defaults to ``target=0``.
"""
callable_bounds_doc = """
    bounds : tuple of {float, ndarray, callable}, optional
        Lower and upper bounds on the objective. Overrides target.
        Both bounds must be broadcastable to to Objective.dim_f
        If a callable, each should take a single argument `rho` and return the
        desired bound (lower or upper) of the profile at those locations.
        Defaults to ``target=0``.
"""

def _unused(doc):
    return doc.rstrip() + " Unused by this objective.\n"


default_doc = target_doc + bounds_doc + weight_doc
profile_doc = callable_target_doc + callable_bounds_doc + weight_doc
dimensionless_doc = target_doc + bounds_doc + _unused(normalize_doc) + _unused(normalize_target_doc)


class MyObjective(_Objective):
    """Do some stuff.

    Parameters
    ----------
    arg1 : type
        arg specific to this objective
    """ + default_doc


class MyProfileObjective(_Objective):
    """Do some stuff.

    Parameters
    ----------
    arg1 : type
        arg specific to this objective
    """ + profile_doc

class DimensionlessObjective(_Objective)
    """Do some stuff.

    Parameters
    ----------
    arg1 : type
        arg specific to this objective
    """ + dimensionless_doc

This could also maybe done automatically via inspection or something with a util function?
One other thing to be careful of is the defaults for target/bounds which are currently in the docstring but may be unique to each objective.

Copy link
Contributor

github-actions bot commented Sep 23, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -0.46 +/- 7.85     | -2.48e-03 +/- 4.23e-02 |  5.36e-01 +/- 2.9e-02  |  5.39e-01 +/- 3.1e-02  |
 test_equilibrium_init_medres            |     -0.75 +/- 2.79     | -3.15e-02 +/- 1.18e-01 |  4.19e+00 +/- 6.5e-02  |  4.22e+00 +/- 9.8e-02  |
 test_equilibrium_init_highres           |     +2.01 +/- 2.18     | +1.10e-01 +/- 1.20e-01 |  5.60e+00 +/- 7.2e-02  |  5.49e+00 +/- 9.6e-02  |
 test_objective_compile_dshape_current   |     +4.84 +/- 5.78     | +1.80e-01 +/- 2.15e-01 |  3.91e+00 +/- 2.5e-02  |  3.73e+00 +/- 2.1e-01  |
 test_objective_compute_dshape_current   |     +0.32 +/- 1.64     | +1.15e-05 +/- 5.89e-05 |  3.61e-03 +/- 4.0e-05  |  3.60e-03 +/- 4.3e-05  |
 test_objective_jac_dshape_current       |     -3.37 +/- 4.84     | -1.39e-03 +/- 2.00e-03 |  3.99e-02 +/- 1.5e-03  |  4.13e-02 +/- 1.3e-03  |
 test_perturb_2                          |     -1.37 +/- 1.61     | -2.49e-01 +/- 2.91e-01 |  1.79e+01 +/- 8.3e-02  |  1.81e+01 +/- 2.8e-01  |
 test_proximal_freeb_jac                 |     -0.04 +/- 1.35     | -2.79e-03 +/- 1.01e-01 |  7.49e+00 +/- 3.8e-02  |  7.49e+00 +/- 9.4e-02  |
 test_solve_fixed_iter                   |     -0.33 +/- 59.10    | -1.65e-02 +/- 2.96e+00 |  5.00e+00 +/- 2.1e+00  |  5.01e+00 +/- 2.1e+00  |
 test_build_transform_fft_midres         |     +2.17 +/- 6.28     | +1.31e-02 +/- 3.79e-02 |  6.17e-01 +/- 3.7e-02  |  6.04e-01 +/- 5.8e-03  |
 test_build_transform_fft_highres        |     +0.55 +/- 1.57     | +5.55e-03 +/- 1.57e-02 |  1.01e+00 +/- 1.4e-02  |  1.00e+00 +/- 6.8e-03  |
 test_equilibrium_init_lowres            |     +0.88 +/- 3.12     | +3.35e-02 +/- 1.18e-01 |  3.82e+00 +/- 1.2e-01  |  3.79e+00 +/- 2.4e-02  |
 test_objective_compile_atf              |     -0.89 +/- 4.08     | -7.06e-02 +/- 3.22e-01 |  7.84e+00 +/- 2.3e-01  |  7.91e+00 +/- 2.3e-01  |
 test_objective_compute_atf              |     +0.46 +/- 1.56     | +4.83e-05 +/- 1.64e-04 |  1.05e-02 +/- 1.4e-04  |  1.05e-02 +/- 9.0e-05  |
 test_objective_jac_atf                  |     +0.21 +/- 3.46     | +4.16e-03 +/- 6.89e-02 |  1.99e+00 +/- 4.4e-02  |  1.99e+00 +/- 5.3e-02  |
 test_perturb_1                          |     +1.12 +/- 2.52     | +1.40e-01 +/- 3.17e-01 |  1.27e+01 +/- 3.1e-01  |  1.26e+01 +/- 6.0e-02  |
 test_proximal_jac_atf                   |     +0.39 +/- 1.02     | +3.21e-02 +/- 8.41e-02 |  8.25e+00 +/- 5.3e-02  |  8.22e+00 +/- 6.5e-02  |
 test_proximal_freeb_compute             |     +0.32 +/- 0.87     | +5.96e-04 +/- 1.62e-03 |  1.85e-01 +/- 7.6e-04  |  1.85e-01 +/- 1.4e-03  |

@YigitElma
Copy link
Collaborator Author

@f0uriest I implemented something similar to what you suggested. If you are fine with it I will make the remaining changes

@YigitElma
Copy link
Collaborator Author

Do we want to use something similar for optimizers and methods of ObjectiveFunction? Those are also the same for parent class and repeated multiple times.

@dpanici
Copy link
Collaborator

dpanici commented Sep 25, 2024

  • have collect_docs take in target_str etc to replace the target string in the same spot in the docstring
  • specific stuff can be at the top, so docstring order might not match init order

@YigitElma YigitElma self-assigned this Sep 25, 2024
@YigitElma YigitElma marked this pull request as ready for review September 25, 2024 20:10
@YigitElma YigitElma requested review from a team, rahulgaur104 and f0uriest and removed request for a team September 25, 2024 20:10
desc/objectives/objective_funs.py Outdated Show resolved Hide resolved
desc/objectives/objective_funs.py Outdated Show resolved Hide resolved
desc/objectives/objective_funs.py Outdated Show resolved Hide resolved
f0uriest
f0uriest previously approved these changes Oct 1, 2024
Copy link
Member

@f0uriest f0uriest left a comment

Choose a reason for hiding this comment

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

Approving, but check with @dpanici about the one that says loss function doesn't do anything

Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

should make relevant changes and mention what we are doing with docstrings in the Adding objectives rst doc file

@YigitElma YigitElma merged commit f0bf028 into master Oct 8, 2024
24 checks passed
@YigitElma YigitElma deleted the yge/docstring branch October 8, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Short and simple to code or review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Objective class args and docstrings
5 participants