-
Notifications
You must be signed in to change notification settings - Fork 229
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
misc: Docstring updates #2223
misc: Docstring updates #2223
Conversation
Looks all good to me. FYI, if you clone the website repo, you can render it locally to check if it looks like what you want |
@@ -163,9 +163,9 @@ def __init__(self): | |||
suffix : str, optional | |||
The JIT compiler version to be used. For example, assuming ``CC=gcc`` and | |||
``suffix='4.9'``, the ``gcc-4.9`` will be used as JIT compiler. | |||
cpp : bool, optional | |||
cpp : bool, optional, default=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this format canonical in numpydoc ?
We never use , defaulat=...
on the first line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devito/types/sparse.py
Outdated
@@ -1148,19 +1148,19 @@ class PrecomputedSparseTimeFunction(AbstractSparseTimeFunction, | |||
So for `r=6`, we will store 18 coefficients per sparse point (instead of | |||
potentially 216). Must be a three-dimensional array of shape | |||
`(npoint, grid.ndim, r)`. | |||
space_order : int, optional | |||
space_order : int, optional, default=0 | |||
Discretisation order for space derivatives. Defaults to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all the "Defaults to ..." in the subsequent lines become redundant so they should maybe be dropped. There are quite a few
@ZoeLeibowitz do you plan on making more change or should this be merged? |
2a785ea
to
f1ed119
Compare
I just pushed some more changes |
Ok thanks, let us know when it's ready |
f1ed119
to
2bfd4d8
Compare
Hi, since I am not 100% sure, I will not make more changes |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2223 +/- ##
==========================================
- Coverage 86.67% 86.56% -0.12%
==========================================
Files 229 229
Lines 43150 43150
Branches 8001 8001
==========================================
- Hits 37401 37352 -49
- Misses 5053 5105 +52
+ Partials 696 693 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZoeLeibowitz could you rebase and push again? in absence of code conflicts, I'd like to merge it
You will have to update the notebooks as well, or some tests will fail.
Have you ever run py.test on our jupyter notebooks locally supplying the --nbval
argument?
ah yes there are now some conflicts in dense.py |
0a86e39
to
6ad769a
Compare
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Ah yes, I think it should be fixed now but could you trigger the CI to check please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks a lot
Thanks, landing! |
Started updating the docstrings for the website. Just want to check I am doing it correctly before I finish them...