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

DOC: api.py, docstring improvements #7732

Merged
merged 3 commits into from
Sep 21, 2021
Merged

Conversation

josef-pkt
Copy link
Member

@josef-pkt josef-pkt commented Sep 20, 2021

more docstrings #7564
and api cleanup in stats #7013
The two issues are not finished by this PR, still some problems left

this includes now some additions to api.rst, and some reordering init by topic

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2021

This pull request introduces 10 alerts when merging 0408b6f into 2e07422 - view on LGTM.com

new alerts:

  • 8 for Unused import
  • 1 for Explicit export is not defined
  • 1 for Implicit string concatenation in a list

@josef-pkt
Copy link
Member Author

Cleanup of module namespaces didn't work (I thought I had done this in the (distant) past)
del names at end of module, deletes them from the runtime environment, and breaks functions
e.g. https://stackoverflow.com/questions/30040658/delete-modules-to-clean-namespace

In my notebook I only imported the modules to check available names. I didn't run any functions. That works without problems.

(initially posted to wrong issue)

@josef-pkt
Copy link
Member Author

checking module name finds imports, e.g.

m = [i for i in m if not getattr(getattr(module, i), "__module__", None) == module.__name__]

but has false positives, if a function is written in another module, but the current module is the "official" public module for it.

@josef-pkt
Copy link
Member Author

josef-pkt commented Sep 21, 2021

I removed all the del at end of modules again, amended and force pushed to clean commit

I still have a few functions missing in api.py for which I'm not sure about whether or how to add them, e.g. knockoff_effects and outlier_influence

@josef-pkt
Copy link
Member Author

related aside: formula.api has del at the end of the module to clean namespace. But it doesn't have functions defined in the module, so there are no problems.

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request introduces 10 alerts when merging ce21816 into 2e07422 - view on LGTM.com

new alerts:

  • 8 for Unused import
  • 1 for Explicit export is not defined
  • 1 for Implicit string concatenation in a list

@josef-pkt
Copy link
Member Author

merge when green

I will do another round later

@bashtage
Copy link
Member

WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.ArchimedeanCopula': no module named statsmodels.distributions.copula.api.ArchimedeanCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.ClaytonCopula': no module named statsmodels.distributions.copula.api.ClaytonCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.CopulaDistribution': no module named statsmodels.distributions.copula.api.CopulaDistribution
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.ExtremeValueCopula': no module named statsmodels.distributions.copula.api.ExtremeValueCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.FrankCopula': no module named statsmodels.distributions.copula.api.FrankCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.GaussianCopula': no module named statsmodels.distributions.copula.api.GaussianCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.GumbelCopula': no module named statsmodels.distributions.copula.api.GumbelCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.IndependenceCopula': no module named statsmodels.distributions.copula.api.IndependenceCopula
WARNING: [autosummary] failed to import 'statsmodels.distributions.copula.api.StudentTCopula': no module named statsmodels.distributions.copula.api.StudentTCopula

@bashtage
Copy link
Member

bashtage commented Sep 21, 2021

I think you need to link to the actual location that is used in the docs, not the API version.

@bashtage
Copy link
Member

Also see

/home/runner/work/statsmodels/statsmodels/docs/source/examples/index.rst:394: WARNING: toctree contains reference to nonexisting document 'examples/notebooks/generated/copula'

Is notebook working correctly?

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request introduces 12 alerts when merging c89bcda into 20082ce - view on LGTM.com

new alerts:

  • 10 for Unused import
  • 1 for Explicit export is not defined
  • 1 for Implicit string concatenation in a list

@josef-pkt
Copy link
Member Author

something strange
notebook and using copula.api in docs works in main
dev html docs look correct

@josef-pkt
Copy link
Member Author

Import error, I made a mistake and didn't run the distribution tests locally.
(I was only paying attention to stats)

@bashtage
Copy link
Member

Can you add an __all__ to get rid of the unused import messages?

@josef-pkt
Copy link
Member Author

problem/type with singular or plural names, I might have to rename a module
depfuncs_ev versus depfunc_ev
for transforms I used plural

@josef-pkt
Copy link
Member Author

@bashtage

why does LGTM ignore pylint and flake options in module?

# pylint: disable=W0611
# flake8: noqa

which satisfies the azure style check in copulas.api

@bashtage
Copy link
Member

Why not just add to __all__ which is the correct approach?

@josef-pkt
Copy link
Member Author

Why not just add to all which is the correct approach?

because it's a pain to maintain and it is never used.

@bashtage
Copy link
Member

bashtage commented Sep 21, 2021 via email

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request introduces 3 alerts when merging bf946d3 into 20082ce - view on LGTM.com

new alerts:

  • 3 for Unused import

@josef-pkt
Copy link
Member Author

another false alarm in docs:

the links to the two other notebooks work correctly in the dev html docs

/home/runner/work/statsmodels/statsmodels/docs/source/examples/notebooks/generated/formulas.ipynb:201: WARNING: File not found: 'examples/notebooks/generated/regression_diagnostics.html'
/home/runner/work/statsmodels/statsmodels/docs/source/examples/notebooks/generated/formulas.ipynb:498: WARNING: File not found: 'examples/notebooks/generated/contrasts.html'

I have one mistake in api.rst, wrong path to model
and one more missing function in stats.api.__all__

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request fixes 8 alerts when merging 6f18ef0 into 20082ce - view on LGTM.com

fixed alerts:

  • 8 for Unused import

@josef-pkt josef-pkt merged commit 5e005e0 into statsmodels:main Sep 21, 2021
@josef-pkt josef-pkt deleted the docs_recent2 branch October 19, 2022 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants