Unify and clean code style and docstring #68

dachengx · 2023-07-30T00:27:36Z

Every time we update the attributes of modules, like adding a module runner, we should run alea/docs/make_docs.sh in alea/docs folder to update alea/docs/reference.

But the docstring of template_source.py is not updated. It will be fixed in #69.

Reference:

Indentation: https://legacy.python.org/dev/peps/pep-0008/#indentation
Docstring format: https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html

This PR tries to apply hanging indentation as much as possible.

You can check the built docs at https://readthedocs.org/projects/alea/builds/ or https://alea--68.org.readthedocs.build/en/68/.

github-actions · 2023-07-30T00:28:51Z

Pull Request Test Coverage Report for Build 5717911329

23 of 29 (79.31%) changed or added relevant lines in 5 files are covered.
7 unchanged lines in 4 files lost coverage.
Overall coverage increased (+1.9%) to 63.193%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
alea/simulators.py	10	12	83.33%
alea/model.py	6	10	60.0%

Files with Coverage Reduction	New Missed Lines	%
alea/models/blueice_extended_model.py	1	94.04%
alea/utils.py	1	78.57%
alea/parameters.py	2	81.25%
alea/model.py	3	59.87%

Totals
Change from base Build 5696617155:	1.9%
Covered Lines:	570
Relevant Lines:	902

💛 - Coveralls

alea/model.py

alea/simulators.py

alea/models/blueice_extended_model.py

alea/examples/gaussian_model.py

alea/simulators.py

alea/model.py

hammannr · 2023-07-31T07:27:47Z

I just had a very brief look. Is there a particular benefit of choosing restructured text docstring? As far as I know, sphinx also supports Google fand NumPy-type docstrings. I generally find reStr text terrible to read and to maintain and if there is no clear point in favor of it I’d much prefer to go to one of the alternative standards.

dachengx · 2023-07-31T12:07:11Z

I just had a very brief look. Is there a particular benefit of choosing restructured text docstring? As far as I know, sphinx also supports Google fand NumPy-type docstrings. I generally find reStr text terrible to read and to maintain and if there is no clear point in favor of it I’d much prefer to go to one of the alternative standards.

OK, I will go to Google style docstring.

alea/model.py

alea/simulators.py

alea/examples/gaussian_model.py

alea/parameters.py

hammannr · 2023-08-01T07:47:30Z

They are here: https://alea--68.org.readthedocs.build/en/68/py-modindex.html @hammannr

Ah very nice! So do we need to make the linking to the things in the sidebar manually? I think this would be very useful!

hammannr

Two minor things and some of the previous discussions are still open but apart from that it looks great -- very nice to have such clean and structured docstrings! 🚀

alea/model.py

dachengx · 2023-08-01T08:32:29Z

They are here: https://alea--68.org.readthedocs.build/en/68/py-modindex.html @hammannr

Ah very nice! So do we need to make the linking to the things in the sidebar manually? I think this would be very useful!

Maybe later. This PR only makes docstring. :)

hammannr

Looks great to me now -- thank you very much for this contribution @dachengx ! 😊

kdund · 2023-08-01T11:55:59Z

alea/examples/gaussian_model.py

+            mu (float, optional (default=None)): mean of the gaussian,
+                if None, the nominal value is used
+            sigma (float, optional (default=None)): standard deviation of the gaussian,
+                if None, the nominal value is used


should we warn that no nominal values are set here?

But if you properly call the ll via .ll instead of ._ll it should parse the defaults or not

Not entirely sure but I think it's possible to run into a useless error if you don't define the defaults and then mu and sigma stay None and are parsed to stats.norm.. Maybe it would make sense to catch this case and provide a more useful error.

We do catch it-- but it is nice to point out this stumbling block in the documentation, in particular since this is a tutorial model, that fails if you use it naively

Updated at https://alea--68.org.readthedocs.build/en/68/reference/alea.examples.html#module-alea.examples.gaussian_model

kdund · 2023-08-01T12:21:57Z

We do catch it, but it is the default behavior of the gaussian model if you just initialize, give it a data set, and try to fit

…

On Tue, Aug 1, 2023, 08:20 Robert Hammann ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In alea/examples/gaussian_model.py <#68 (comment)>: > if parameter_definition is None: parameter_definition = ["mu", "sigma"] super().__init__(parameter_definition=parameter_definition, **kwargs) def _ll(self, mu=None, sigma=None): + """ + Log-likelihood of the model. + + Args: + mu (float, optional (default=None)): mean of the gaussian, + if None, the nominal value is used + sigma (float, optional (default=None)): standard deviation of the gaussian, + if None, the nominal value is used Not entirely sure but I think it's possible to run into a useless error if you don't define the defaults and then mu and sigma stay None and are parsed to stats.norm.. Maybe it would make sense to catch this case and provide a more useful error. — Reply to this email directly, view it on GitHub <#68 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEAPFKJWFAGTG45RHEBY5NLXTDX7FANCNFSM6AAAAAA24Z7C6Y> . You are receiving this because your review was requested.Message ID: ***@***.***>

hammannr · 2023-08-01T12:28:55Z

Ah, right @kdund we catch it in the Parameters class. Then I think everything is fine or not? Probably I'm just missing your point

kdund

Hi! Few minor changes-- I think we should warn that the gaussian model must have nominal values set manually, or fitting will not work (i.e. write it in the docstring) and we should label the likelihood likelihood not logpdf in the example placeholder

kdund · 2023-08-01T12:06:44Z

alea/model.py

        raise NotImplementedError(
            "You must write a data-generation method (_generate_data) for your statistical model"
            " or use a subclass where it is written for you")

    @_needs_data
    def ll(self, **kwargs) -> float:
        """
-        Likelihod function, returns the loglikelihood for the given parameters.
+        Likelihod function, return the loglikelihood for the given parameters.


returns is correct

kdund · 2023-08-01T12:08:49Z

alea/model.py

+        Make a function that can be passed to Minuit
+
+        Args:
+            minus (bool, optional (default=True)): if True, the function is multiplied by -1


why have minus here if we never use the option?

Do you mean that we drop this minus and always use True? Maybe @hammannr can comment.

Yes, good point -- let's drop it. It's a remnant from blueice, we don't need this flexibility here I think.

kdund · 2023-08-01T12:09:26Z

alea/model.py

        """
        Generate data for the given parameters.
        The parameters are passed as keyword arguments, positional arguments are not possible.
-        If a parameter is not given, the default value is used.


I think I would keep singular to emphasize that it is the default value singular for that parameter (i.e. not for all parameters)

kdund · 2023-08-01T14:25:51Z

alea/examples/gaussian_model.py

+            mu (float, optional (default=None)): mean of the gaussian,
+                if None, the nominal value is used
+            sigma (float, optional (default=None)): standard deviation of the gaussian,
+                if None, the nominal value is used


We do catch it-- but it is nice to point out this stumbling block in the documentation, in particular since this is a tutorial model, that fails if you use it naively

kdund · 2023-08-01T14:30:57Z

alea/simulators.py

        r_data = np.zeros(np.sum(n_sources), dtype=self.dtype)
        i_write = 0
        for i, n_source in enumerate(n_sources):
-            if 0 < n_source:  #dont generate if 0
+            if n_source > 0:  # dont generate if 0


This is the wrong way, no? Always better to go smaller->larger

PEP8's WPS309 prefer putting variable first: #68 (comment)

In that case, I want us to ignore WPS309 errors.
To me this is a significant readability issue.

I see your reasoning Knut but I have to say I'm so used to it that writing 0 < n_sources takes me a few seconds to understand 😄 Though I think it's a small detail and I can live with either solution.

kdund · 2023-08-01T14:33:04Z

alea/utils.py

@@ -42,8 +41,7 @@ def adapt_likelihood_config_for_blueice(

    Args:
        likelihood_config (dict): likelihood config dict
-        template_folder_list (list): list of possible base folders.
-            Ordered by priority.
+        template_folder_list (list): list of possible base folders. Ordered by priority.


which order? Try first one first or last one first?

kdund · 2023-08-01T14:35:55Z

notebooks/placeholder.ipynb

While "logPDF" is correct in principle here, I would label it likelihood instead since it is used as a function of mu, rather than the observation.

actually, so it is not correct in principle, but in practise :)

…calModel.make_objective

Add reference of modules indices

51eceb8

Update docstring of StatisticalModel

ada2cdc

github-actions bot reviewed Jul 30, 2023

View reviewed changes

alea/model.py Outdated Show resolved Hide resolved

alea/model.py Outdated Show resolved Hide resolved

alea/model.py Show resolved Hide resolved

Update docstring of BlueiceExtendedModel

274be84

github-actions bot reviewed Jul 30, 2023

View reviewed changes

alea/simulators.py Outdated Show resolved Hide resolved

alea/simulators.py Outdated Show resolved Hide resolved

alea/simulators.py Outdated Show resolved Hide resolved

alea/simulators.py Outdated Show resolved Hide resolved

alea/models/blueice_extended_model.py Show resolved Hide resolved

Update docstring of GaussianModel

8e69318

github-actions bot reviewed Jul 30, 2023

View reviewed changes

alea/examples/gaussian_model.py Show resolved Hide resolved

dachengx added 2 commits July 30, 2023 10:52

Update docstring of Parameters

fe72dc9

Update docstring of BlueiceExtendedModel

3c6f281

github-actions bot reviewed Jul 30, 2023

View reviewed changes

alea/simulators.py Outdated Show resolved Hide resolved

alea/simulators.py Show resolved Hide resolved

alea/simulators.py Show resolved Hide resolved

alea/simulators.py Show resolved Hide resolved

More code style change

ee0da2f

github-actions bot reviewed Jul 31, 2023

View reviewed changes

alea/model.py Outdated Show resolved Hide resolved

Try simplified name

de83227

github-actions bot reviewed Jul 31, 2023

View reviewed changes

alea/model.py Outdated Show resolved Hide resolved

Change it back

2e4c06c

dachengx force-pushed the code_docstring_style branch from 7355bd4 to 2e4c06c Compare July 31, 2023 00:42

dachengx marked this pull request as ready for review July 31, 2023 00:47

dachengx added 5 commits July 30, 2023 19:57

Add codes to placeholder.ipynb

0237c1f

Recover symbolic link

617fa8b

Minor change

e9515c4

Try plot the logpdf

6772eca

Update odcstring of utils

d4e0f20

dachengx requested review from hammannr and kdund July 31, 2023 06:11

dachengx added 2 commits July 31, 2023 07:17

Use napoleon and todo as sphinx extension

7de6852

Update model.py docstring to Google style

908b49d

github-actions bot reviewed Jul 31, 2023

View reviewed changes

alea/model.py Outdated Show resolved Hide resolved

Update docstrings

d834e81

dachengx requested a review from hammannr July 31, 2023 17:34

Minor change

0311397

github-actions bot reviewed Jul 31, 2023

View reviewed changes

dachengx added 3 commits July 31, 2023 12:46

Minor change, drop DAR105

90b86c2

Minor change

0588fe9

Only check code style when PR opened

7f5657b

github-actions bot reviewed Jul 31, 2023

View reviewed changes

alea/parameters.py Show resolved Hide resolved

alea/parameters.py Show resolved Hide resolved

This was referenced Jul 31, 2023

Simplify TemplateSource, CombinedSource and SpectrumTemplateSource #69

Merged

First runner manipulating statistical model #50

Merged

hammannr reviewed Aug 1, 2023

View reviewed changes

alea/model.py Outdated Show resolved Hide resolved

alea/model.py Outdated Show resolved Hide resolved

dachengx added 2 commits August 1, 2023 03:34

Minor fix

10b8d97

Update docstring to tell the relation between public and private method

4529a7f

dachengx requested a review from hammannr August 1, 2023 08:46

Remove kwargs from make_objective

01bb5ed

hammannr approved these changes Aug 1, 2023

View reviewed changes

kdund reviewed Aug 1, 2023

View reviewed changes

kdund requested changes Aug 1, 2023

View reviewed changes

Update docstrings of GaussianModel, remove minus argument of Statisti…

702353b

…calModel.make_objective

dachengx requested a review from kdund August 1, 2023 16:15

Debug

42e4d7f

kdund approved these changes Aug 1, 2023

View reviewed changes

Minor change

2682d9f

dachengx merged commit 58bf303 into main Aug 1, 2023

dachengx deleted the code_docstring_style branch August 1, 2023 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify and clean code style and docstring #68

Unify and clean code style and docstring #68

dachengx commented Jul 30, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2023 •

edited

Loading

hammannr commented Jul 31, 2023 •

edited

Loading

dachengx commented Jul 31, 2023

hammannr commented Aug 1, 2023

hammannr left a comment

dachengx commented Aug 1, 2023

hammannr left a comment

kdund Aug 1, 2023

hammannr Aug 1, 2023

hammannr Aug 1, 2023

kdund Aug 1, 2023

dachengx Aug 1, 2023

kdund commented Aug 1, 2023 via email

hammannr commented Aug 1, 2023

kdund left a comment

kdund Aug 1, 2023

kdund Aug 1, 2023

dachengx Aug 1, 2023

hammannr Aug 1, 2023

kdund Aug 1, 2023

kdund Aug 1, 2023

kdund Aug 1, 2023

dachengx Aug 1, 2023

kdund Aug 1, 2023 •

edited

Loading

hammannr Aug 1, 2023

kdund Aug 1, 2023

kdund Aug 1, 2023

kdund Aug 1, 2023

Unify and clean code style and docstring #68

Unify and clean code style and docstring #68

Conversation

dachengx commented Jul 30, 2023 • edited Loading

github-actions bot commented Jul 30, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5717911329

💛 - Coveralls

hammannr commented Jul 31, 2023 • edited Loading

dachengx commented Jul 31, 2023

hammannr commented Aug 1, 2023

hammannr left a comment

Choose a reason for hiding this comment

dachengx commented Aug 1, 2023

hammannr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdund commented Aug 1, 2023 via email

hammannr commented Aug 1, 2023

kdund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdund Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachengx commented Jul 30, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2023 •

edited

Loading

hammannr commented Jul 31, 2023 •

edited

Loading

kdund Aug 1, 2023 •

edited

Loading