-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
documentation building, readthedocs config #203
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #203 +/- ##
==========================================
+ Coverage 95.59% 95.74% +0.15%
==========================================
Files 12 12
Lines 1905 1905
==========================================
+ Hits 1821 1824 +3
+ Misses 84 81 -3 ☔ View full report in Codecov by Sentry. |
This PR to update the docs is now at the state of being incomplete but reviewable. By way of documentation for what has changed in this PR:
I am fine with using readthedocs. But, I am also willing to use GitHub pages. I use this for almost all my other projects, and it has the distinct advantage of allowing us to put any files we want in the gh-pages branch, including static PDFs. By way of proof-of-concept and viewing the development here, I added a gh-pages branch to the main We could use both, and/or make the links to the PDF and zipped HTML point to the fully-qualified URLs https://lmfit.github.io/uncertainties/uncertainties.pdf and https://lmfit.github.io/uncertainties/uncertainties_doc.zip. I think there is always more to do. And I would say that all the changes here could be undone or modified: suggestions most welcome. |
doc/user_guide.rst
Outdated
|
||
.. The "import uncertainties" is put here because some examples requires | ||
uncertainties to have been imported (and not only ufloat). | ||
>>> import uncertainties as un |
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.
does this conflict with any other packages?
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.
I think it does not but have no idea how to tell!
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.
dunno. One could probably ask that about any abbreviation.
the table of contents has switched position with 'get uncertainties' on index.html |
I'd remove the autofuncton then this looks good to merge |
Yes. The intention is that the information on how to get uncertainties is prominent on the index page.
Do you mean you want to not have the docstrings for I think these autofunctions could be moved, but I think they should be given somewhere. |
That's fine, the 'get uncertainties' on the box on the right where the toc
would be looks odd
I mean to move them to another page. Personally I'd leave that page till a
later PR when there's more updated doc strings, but you could do so in this
PR.
…On Tue, 23 Apr 2024, 03:21 Matt Newville, ***@***.***> wrote:
@andrewgsavage <https://github.com/andrewgsavage>
the table of contents has switched position with 'get uncertainties' on
index.html
Yes. The intention is that the information on how to get uncertainties is
prominent on the index page.
Is this a problem?
I'd remove the autofuncton then this looks good to merge
Do you mean you want to not have the docstrings for ufloat and
ufloat_fromstr? One of the (okay, many) things I find odd about the
current doc is that these are not provided. And the docstrings are kind of
a mess.
I think these autofunctions could be moved, but I think they should be
given somewhere.
—
Reply to this email directly, view it on GitHub
<#203 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEMLEGPZRIGT3TBN5DX5B3Y6XAUHAVCNFSM6AAAAABEXEZDVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGI4DQOBYGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
doc/user_guide.rst
Outdated
|
||
Each number created this way is an **independent (random) variable** | ||
(for details, see the :ref:`Technical Guide <math_def_num_uncert>`). | ||
.. autofunction:: ufloat |
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.
The section created from the docstring is quite long, making this page more difficult to read for a new user.
I'd prefer if it linked to the function in an API reference section
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.
That seems OK. The current docs don't describe Variable, UFloat, or AffineScalarFunc. They sort of talk about "value with an uncertainty". I think that an API section really should mention this. Opinions?
doc/user_guide.rst
Outdated
|
||
.. The "import uncertainties" is put here because some examples requires | ||
uncertainties to have been imported (and not only ufloat). | ||
>>> import uncertainties as un |
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.
I think it does not but have no idea how to tell!
used. Running the test suite requires `pytest` and `pytest_cov`, and building | ||
these docs requires `sphinx`. To install these optional packages, use one of: | ||
|
||
.. code-block:: sh |
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.
This is very helpful, thanks for adding
doc/formatting.rst
Outdated
>>> print('Result = {:10.2f}'.format(x)) | ||
Result = 0.20+/- 0.01 | ||
|
||
(Python 2.6 requires ``'{0:10.2f}'`` instead, with the usual explicit |
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.
python2 reference can be deleted
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.
hah! thanks - I missed that one.
doc/formatting.rst
Outdated
|
||
>>> print('Automatic number of digits on the uncertainty: {}'.format(x)) | ||
Automatic number of digits on the uncertainty: 0.200+/-0.010 | ||
>>> print(x0 |
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.
close bracket
might be worth setting these up as doc tests in the future
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.
+1
doc/user_guide.rst
Outdated
|
||
.. The "import uncertainties" is put here because some examples requires | ||
uncertainties to have been imported (and not only ufloat). | ||
>>> import uncertainties |
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.
single space before >>>
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.
On second read, I think this (lines 19-23) can be removed. Especially if this is to become 'getting started'
It doesn't add anything; new users need to read through this page and will get to adv math functions. Numpy users can find that easily.
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.
Yep, I agree...
doc/user_guide.rst
Outdated
Basic math with uncertain Variables | ||
========================================= | ||
|
||
:class:`Variable`s can be used in basic mathematical calculations |
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.
:class:[
](https://lmfit.github.io/uncertainties/user_guide.html#id6)Variable
s
hasn't rendered correctly
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.
will investigate (when moved to an api.rst
, probably)
doc/user_guide.rst
Outdated
the functions from the standard** :mod:`math` **module**. These | ||
mathematical functions are found in the :mod:`uncertainties.umath` | ||
module: | ||
Besides being able to apply basic mathematical operations to uncertaintties |
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.
uncertaintties > uncertainties
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.
+1
doc/user_guide.rst
Outdated
|
||
>>> print(x*y) | ||
240.0+/-76.83749084919418 | ||
>>> (x*y).std_dev == (x*y).nominal_value * sqrt((x.std_dev/x.nominal_value)**2 + (y.std_dev/y.nominal_value)**2 ) |
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.
I'd introduce the .n and .s abbreviations early on (maybe at line 50?) and use them here to make this shorter.
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.
OK, I was thinking maybe avoid .n
and .s
as they might be something to deprecate. I'm OK with fully embracing them as part of the API.
uncertainties/core.py
Outdated
Objects are meant to represent variables that are independent from | ||
each other (correlations are handled through the AffineScalarFunc | ||
class). | ||
Variablees are independent from each other, but correlations between them |
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.
Variablees >Variables
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.
+1
Looks good, how much more are you planning to do on this before merging? this + changelog are the last things to do before a new release. |
Yeah, how much more to do is a great question ;). I might ask opinions all around (@wshanks, @jagerber48) on how complete this needs to be, and if the version at https://lmfit.github.io/uncertainties is approaching "acceptable". |
I think the main thing is to remove anything python2 related + typos/formatting misses. Anything more is nice to have and can be done in a later PR. It looks like you're roughly halfway through editing each of the files, with numpy and tech guide to go? Might as well continue through those. I'd leave converting user guide -> getting started out of this PR to make it easier to review what's moved where and ensure nothings getting removed. There's enough moving in this PR already to keep track of! |
OK, sorry for the delay on this. I believe I have addressed most of the issues and this might be good enough, at least for a while. Suggestions? Again, rendered results are at https://lmfit.github.io/uncertainties |
doc/install.rst
Outdated
The :mod:`uncertainties` package was written and developed by `Eric O. LEBIGOT | ||
(EOL)`_. EOL also maintained the package until 2024, when the GitHub project | ||
was moved to the `lmfit GitHub organization`_ to allow more sustainable | ||
development and maintenance. Currentt members of the devlopment and |
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.
Current
doc/user_guide.rst
Outdated
|
||
>>> import uncertainties | ||
To create a number with uncertainties or *Variable*, use the :func:`ufloat` | ||
function, which takes a *nominal value* ( (which can be interpreted as the most |
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.
Double open bracket
doc/user_guide.rst
Outdated
To create a number with uncertainties or *Variable*, use the :func:`ufloat` | ||
function, which takes a *nominal value* ( (which can be interpreted as the most | ||
likely value, or the mean or central value of the distribution of values), a | ||
*standard error* ( (the standard deviation or :math:`1-\sigma` uncertainty), and |
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.
Double open bracket
doc/user_guide.rst
Outdated
complicated. That is, a Variable with a value of 25 +/- 10 might be greater | ||
than a Variable with a value of 24 +/- 8 most of the time, but *sometimes* it | ||
might be less than it. The :mod:`uncertainties` package takes the simple | ||
approach of comparing. That is |
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.
Of comparing nominal values.
There's some python 2 print statements in the tech guide |
Also some python 2 print statements on the bumpy guide |
Also some python 2 print statements on the numpy guide |
Numpy* For Python 2: converters = dict.fromkeys(range(num_cols), uncertainties.ufloat_fromstr) Other than that this looks and reads well. Id fix those then merge. |
@andrewgsavage Thanks!! OK, I think I have addressed hose spelling/print errors. I am still investigating the "loadtxt" thing: "The code does what???" |
@andrewgsavage OK, I think the loadtxt/savetxt doc is correct now. |
|
||
.. Formatting method: | ||
|
||
More **control over the format** can be obtained (in Python 2.6+) |
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.
Python2 reference
@andrewgsavage Thanks! Should we re-kindle the conversation about tagging and releasing 3.2.0? |
Looks good, merged since there's been no indication anyone else will read through since you asked 2 weeks ago. |
yea please |
- [x] Closes #270 - [x] Executed `pre-commit run --all-files` with no errors - [x] The change is fully covered by automated unit tests - [x] Documented in docs/ as appropriate - [x] Added an entry to the CHANGES file This PR hopes to resolve a few issues with `uncertainty` package interaction with `codecov.io`. - #203 introduced `.codecov.yaml`. Unfortunately, I think this was an invalid configuration. Specifically because the `comment` section is supposed to be a top-level section, not nested under the `codecov` section. This means that since that PR the yaml configuration has NOT been used for our codecov updates. I think this explains why we're seeing codecov failures despite best attempts to suppress them. - See for example #268 where a codecov failure is creating an X when we really just want the PR to go through. This PR will (attempt to) make it so that codecov coverage is reported, but will never result in a failed coverage run. This way reviewers can see if patch coverage is < 100% or if overall coverage drops due to a PR. If reviewers deem a lack or drop of coverage to be significant they can block PR approval. Otherwise, for minor coverage drops, the coverage can be easily ignored. It does so using the `informational` flag in the `.codecov.yaml` file. - It moves the API token back to using the github secret rather than exposing it in the `.codecov.yaml` file.
this is a work-in-progress to start on getting docs to build and push to readthedocs.
so far, it has not touched the doc contents, but some of that will need changing as well.