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 docstrings to adjoint solver #2556

Merged
merged 5 commits into from
Jun 22, 2023
Merged

add docstrings to adjoint solver #2556

merged 5 commits into from
Jun 22, 2023

Conversation

mochen4
Copy link
Collaborator

@mochen4 mochen4 commented Jun 21, 2023

Add docstrings to objective.py and optimization_problem.py

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2023

Codecov Report

Merging #2556 (499c8f8) into master (37516cf) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #2556      +/-   ##
==========================================
- Coverage   73.91%   73.90%   -0.01%     
==========================================
  Files          18       18              
  Lines        5294     5293       -1     
==========================================
- Hits         3913     3912       -1     
  Misses       1381     1381              
Impacted Files Coverage Δ
python/adjoint/optimization_problem.py 59.48% <ø> (ø)
python/adjoint/objective.py 92.38% <100.00%> (-0.04%) ⬇️

... and 2 files with indirect coverage changes

@@ -284,11 +293,30 @@ def __call__(self):


class FourierFields(ObjectiveQuantity):
"""A differentiable frequency-dependent Fourier Fields (dft_fields)
Attributes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace "Attributes:" with "Args:" and separate it with the one-line summary above it with a line break following the style guide: https://google.github.io/styleguide/pyguide.html#s3.8.3-functions-and-methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought for classes, it should be attributes instead of args according to https://google.github.io/styleguide/pyguide.html#s3.8.4-comments-in-classes? And that was how Ian wrote the docstrings for EigenmodeCoefficient previously in #1593.
On the other hand, I am confused about whether to put the docstrings in class or __init__. Based on https://stackoverflow.com/questions/37019744/is-there-a-consensus-on-what-should-be-documented-in-the-class-and-init-docs, it seems to that they should be in class based on an earleir version of google style guide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Attributes is used for classes and Args for functions.

For the Attributes section of the docstrings for the EigenmodeCoefficient class, we would need to list its public members, almost anything with a self. prefix defined in the constructor excluding self._ which are generally used for private members.

We should also add a separate Args section to the constructor. The argument descriptions in the constructor docstrings should hopefully be different than the attributes description in the class docstring. The attributes description should be a general summary whereas the argument description is less so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added separate Args sections to the constructors. I tried to make the attribute description more concise and general, and argument description more detailed and elaborated. But they still seem a little redundant to me.

python/adjoint/objective.py Outdated Show resolved Hide resolved
python/adjoint/objective.py Outdated Show resolved Hide resolved
python/adjoint/objective.py Outdated Show resolved Hide resolved
python/adjoint/objective.py Outdated Show resolved Hide resolved
python/adjoint/optimization_problem.py Outdated Show resolved Hide resolved
A tuple (f0, gradient) where:
f0 is the list of objective functions values when design variables
are set to rho_vector
gradient is the lists of gradients of objective functions with respect to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clarify that gradient is a list (over objective functions) of lists (over design regions) of 2d arrays (design vars x frequencies) of derivatives. If there is only a single objective function, the outer 1-element list is replaced by just that element, and similarly if there is only one design region, while if there is only 1 frequency then the innermost array is squeezed to a 1d array. (So, for example, if you have only a single objective function, a single design region, and a single frequency, then gradient is simply a 1d array of the derivatives.)

@@ -21,6 +21,26 @@ class OptimizationProblem:
calculation) and optionally its gradient (adjoint calculation).
This is done by the __call__ method.

Attributes:
sim: the corresponding Meep `Simulation` object of the problem
objective_functions: list of objective functions on objective_arguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
objective_functions: list of objective functions on objective_arguments
objective_functions: list of differentiable functions (callable objects) whose arguments are given by objective_arguments

@mochen4 mochen4 marked this pull request as ready for review June 22, 2023 20:19
@stevengj stevengj merged commit 95ddacd into NanoComp:master Jun 22, 2023
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.

4 participants