Edit docstring in `run_tardis` #1723

atharva-2001 · 2021-07-15T12:23:42Z

This edits the docstring in run_tardis function.

Description

The docstrings in run_tardis functions are incomplete and need to be edited.

Motivation and context

Resolves #1719

How has this been tested?

Testing pipeline.
Other.

Examples

Link to the API documentation

Type of change

Bug fix.
New feature.
Breaking change.
None of the above.

Checklist

My change requires a change to the documentation.
- I have updated the documentation accordingly.
- (optional) I have built the documentation on my fork following the instructions.
I have assigned and requested two reviewers for this pull request.

codecov · 2021-07-15T12:35:33Z

Codecov Report

Merging #1723 (e102585) into master (2c47baa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1723   +/-   ##
=======================================
  Coverage   61.88%   61.88%           
=======================================
  Files          63       63           
  Lines        5851     5851           
=======================================
  Hits         3621     3621           
  Misses       2230     2230

Impacted Files	Coverage Δ
tardis/tardis/base.py	`57.69% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c47baa...e102585. Read the comment docs.

marxwillia · 2021-07-15T14:37:44Z

tardis/base.py

+    log_state : str, optional
+        Set the level of the TARDIS logger. (Default level of the logger is set to CRITICAL.)
+    specific : bool, optional
+        If True, only show the log messages from a particular log level, set by `log_state`.


Explain what happens if set to False.

marxwillia · 2021-07-15T14:38:01Z

tardis/base.py

+    virtual_packet_logging : bool, default False, optional
+        Option to enable virtual packet logging.
+    log_state : str, optional
+        Set the level of the TARDIS logger. (Default level of the logger is set to CRITICAL.)


List options for this argument.

marxwillia · 2021-07-15T14:40:02Z

tardis/base.py

@@ -1,4 +1,4 @@
-# functions that are important for the general usage of TARDIS
+"""Functions that are important for the general usage of TARDIS."""


let's keep this as a regular comment.

tardis-bot · 2021-07-15T17:11:35Z

Before a pull request is accepted, it must meet the following criteria:

Is the necessary information provided?
Is this a duplicate PR?
- If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
Does it pass existing tests and are new tests provided if required?
- The test coverage should not decrease, and for new features should be close to 100%.
Is the code tidy?
- No unnecessary print lines or code comments.

jaladh-singhal

Thanks for doing this necessary thing - there are few things I've commented.

Also make sure to get this merged first and then rebase #1636

jaladh-singhal · 2021-07-15T18:03:38Z

tardis/base.py

+        [[callback1, callback_arg1], [callback2, callback_arg2], ...]
+        Where the callback function signature should look like:
+        callback_function(simulation, extra_arg1, ...)
+    virtual_packet_logging : bool, default False, optional


Not sure if you can write default values in parameter type, they should be in parameter description AFAIR

I'm afraid the examples mentioned here have the default values next to parameter type, that's why I used it.
I didn't follow that convention in #1636 and I will fix it ASAP. Pandas also follows the NumPy style guide, it seems they did the same too.

Oh, I see so numpydoc says both are valid (in type or in description). Thanks for linking up examples

If you're keeping it in type then do it throughout the module - when there are options we should follow one option consistently since that will enhance readability

I agree with Jaladh. We should have consistent docstring formats throughour the whole Project.

default: False like you did below

jaladh-singhal · 2021-07-15T18:04:25Z

tardis/base.py

+    log_state : str, optional
+        Set the level of the TARDIS logger. Options include `NOTSET`, `DEBUG`, `INFO`, `WARNING`, `ERROR`, and `CRITICAL`.
+        Default level of the logger is set to CRITICAL.
+    specific : bool, optional


See this - you are also breaking consistency by mentioning some default values in parameter type - let's keep all in description

The default value of the argument in the function is None, but that of the logger is actually CRITICAL, which is set here I believe. I guess moving this to the parameter section would mean that the default argument is CRITICAL, but it's actually None.

Maybe it would be a good idea to mention that the default value is set here?

Then add default: None here and explain in description what does none imply

tardis/base.py

DhruvSondhi

It looks better than before but I have some concerns regarding the log_state argument definition.

tardis/base.py

jaladh-singhal

Thanks for updates. These are small change requests but please pay attention to details - you're missing the point of consistency that I mentioned earlier (so that you don't have to redo it 😄 )

Please let me know if there are any doubts

jaladh-singhal · 2021-07-16T12:02:10Z

tardis/base.py

+        The default value of the argument is `None` but the default level
+        of the logger is `CRITICAL`.


This confusing to even me. You just need to tell what happens to logging when this parameter is set to None, let me know if it is still unclear?
@DhruvSondhi can you help him out?

This is actually correct because the default value is being set via the YAML parameter. It is a little complicated. @jaladh-singhal please do discuss this with me.

tardis/base.py

jaladh-singhal · 2021-07-16T12:06:51Z

tardis/base.py

+    packet_source : class, optional
+        A custom packet source class or a child class of `tardis.montecarlo.packet_source`
+        used to override the TARDIS `BasePacketSource` class.
+    simulation_callbacks : list of lists, optional


Default value missing in type, also we need description what empty list does

tardis/base.py

jaladh-singhal · 2021-07-16T12:09:18Z

tardis/base.py

+        [[callback1, callback_arg1], [callback2, callback_arg2], ...]
+        Where the callback function signature should look like:
+        callback_function(simulation, extra_arg1, ...)
+    virtual_packet_logging : bool, default False, optional


default: False like you did below

DhruvSondhi · 2021-07-19T06:09:52Z

Can you please share the preview link for the documentation?

atharva-2001 · 2021-07-19T06:23:27Z

Sure, could you please check this out?

jaladh-singhal

Looks good to me - you just need to rebase it and rename log_state to log_level (#1730)

* edit docstring in run_tardis * [build docs]

atharva-2001 added the documentation label Jul 15, 2021

marxwillia requested changes Jul 15, 2021

View reviewed changes

atharva-2001 force-pushed the run_tardis_docstring branch from 314a6f5 to 3cd8c61 Compare July 15, 2021 17:11

atharva-2001 marked this pull request as ready for review July 15, 2021 17:11

atharva-2001 requested review from marxwillia, jaladh-singhal and DhruvSondhi July 15, 2021 17:11

atharva-2001 force-pushed the run_tardis_docstring branch from 3cd8c61 to d3f2d79 Compare July 15, 2021 17:15

jaladh-singhal requested changes Jul 15, 2021

View reviewed changes

jaladh-singhal reviewed Jul 15, 2021

View reviewed changes

tardis/base.py Outdated Show resolved Hide resolved

atharva-2001 force-pushed the run_tardis_docstring branch 2 times, most recently from e2674f0 to ba488dd Compare July 15, 2021 19:46

DhruvSondhi suggested changes Jul 16, 2021

View reviewed changes

tardis/base.py Outdated Show resolved Hide resolved

atharva-2001 force-pushed the run_tardis_docstring branch from ba488dd to ca82574 Compare July 16, 2021 11:19

atharva-2001 requested review from jaladh-singhal and DhruvSondhi July 16, 2021 11:20

jaladh-singhal requested changes Jul 16, 2021

View reviewed changes

atharva-2001 force-pushed the run_tardis_docstring branch from ca82574 to 915a791 Compare July 19, 2021 05:43

marxwillia approved these changes Jul 22, 2021

View reviewed changes

atharva-2001 requested a review from jaladh-singhal July 22, 2021 19:00

jaladh-singhal reviewed Jul 22, 2021

View reviewed changes

atharva-2001 added 2 commits July 23, 2021 00:45

edit docstring in run_tardis

ea8c499

[build docs]

e102585

atharva-2001 force-pushed the run_tardis_docstring branch from 915a791 to e102585 Compare July 22, 2021 19:15

atharva-2001 requested a review from jaladh-singhal July 22, 2021 19:16

jaladh-singhal approved these changes Jul 22, 2021

View reviewed changes

jaladh-singhal merged commit 2010e81 into tardis-sn:master Jul 22, 2021

jaladh-singhal mentioned this pull request Jul 22, 2021

Adding Convergence Plots #1636

Merged

10 tasks

DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 1, 2021

Edit docstring in run_tardis (tardis-sn#1723)

201d0fe

* edit docstring in run_tardis * [build docs]

DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021

Edit docstring in run_tardis (tardis-sn#1723)

45288f0

* edit docstring in run_tardis * [build docs]

DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021

Edit docstring in run_tardis (tardis-sn#1723)

a70f659

* edit docstring in run_tardis * [build docs]

atharva-2001 added a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021

Edit docstring in run_tardis (tardis-sn#1723)

6ea8368

* edit docstring in run_tardis * [build docs]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit docstring in `run_tardis` #1723

Edit docstring in `run_tardis` #1723

atharva-2001 commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

marxwillia Jul 15, 2021

marxwillia Jul 15, 2021

marxwillia Jul 15, 2021

tardis-bot commented Jul 15, 2021

jaladh-singhal left a comment

jaladh-singhal Jul 15, 2021

atharva-2001 Jul 15, 2021 •

edited

Loading

jaladh-singhal Jul 15, 2021

DhruvSondhi Jul 15, 2021 •

edited

Loading

jaladh-singhal Jul 16, 2021

jaladh-singhal Jul 15, 2021

atharva-2001 Jul 15, 2021 •

edited

Loading

jaladh-singhal Jul 15, 2021

DhruvSondhi left a comment

jaladh-singhal left a comment •

edited

Loading

jaladh-singhal Jul 16, 2021 •

edited

Loading

DhruvSondhi Jul 16, 2021 •

edited

Loading

jaladh-singhal Jul 16, 2021

jaladh-singhal Jul 16, 2021

DhruvSondhi commented Jul 19, 2021

atharva-2001 commented Jul 19, 2021 •

edited

Loading

jaladh-singhal left a comment

		@@ -1,4 +1,4 @@
		# functions that are important for the general usage of TARDIS
		"""Functions that are important for the general usage of TARDIS."""

		The default value of the argument is `None` but the default level
		of the logger is `CRITICAL`.

Edit docstring in run_tardis #1723

Edit docstring in run_tardis #1723

Conversation

atharva-2001 commented Jul 15, 2021 • edited Loading

codecov bot commented Jul 15, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tardis-bot commented Jul 15, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-2001 Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhruvSondhi Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-2001 Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhruvSondhi left a comment

Choose a reason for hiding this comment

jaladh-singhal left a comment • edited Loading

Choose a reason for hiding this comment

jaladh-singhal Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

DhruvSondhi Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhruvSondhi commented Jul 19, 2021

atharva-2001 commented Jul 19, 2021 • edited Loading

jaladh-singhal left a comment

Choose a reason for hiding this comment

Edit docstring in `run_tardis` #1723

Edit docstring in `run_tardis` #1723

atharva-2001 commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

atharva-2001 Jul 15, 2021 •

edited

Loading

DhruvSondhi Jul 15, 2021 •

edited

Loading

atharva-2001 Jul 15, 2021 •

edited

Loading

jaladh-singhal left a comment •

edited

Loading

jaladh-singhal Jul 16, 2021 •

edited

Loading

DhruvSondhi Jul 16, 2021 •

edited

Loading

atharva-2001 commented Jul 19, 2021 •

edited

Loading