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

MAINT: Distinguish between private and public namespaces #601

Merged
merged 14 commits into from
Dec 17, 2022

Conversation

Smit-create
Copy link
Member

@Smit-create Smit-create commented Feb 11, 2022

Related to #350

I am trying to work on #350 by using private and public namespace changes. Since ecdf.py is not meant for public namespace i.e we want users to use from quantecon import ECDF instead of from quantecon.ecdf import ECDF, we can do the following to resolve this:

  1. Rename ecdf.py to _ecdf.py using git mv (This preserves the commit history of ecdf.py and transfers it to _ecdf.py)
  2. Create a new file ecdf.py and add warnings (This is required at the current stage to avoid breaking the code and can be removed safely in future versions)

The current PR works in the following way:

>>> from quantecon.ecdf import ECDF  # works but throws a warning
<stdin>:1: DeprecationWarning: Please use `ECDF` from the `quantecon` namespace, the `quantecon.ecdf` namespace is deprecated.
>>> from quantecon import ECDF # works fine

We can remove the ecdf.py file in future versions and then we will just have _ecdf.py as a private namespace

cc @mmcky @jstac

@pep8speaks
Copy link

pep8speaks commented Feb 11, 2022

Hello @Smit-create! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 31:1: W191 indentation contains tabs
Line 31:7: E101 indentation contains mixed spaces and tabs
Line 31:8: E128 continuation line under-indented for visual indent
Line 33:1: W191 indentation contains tabs
Line 33:7: E101 indentation contains mixed spaces and tabs
Line 33:9: E128 continuation line under-indented for visual indent
Line 53:1: E265 block comment should start with '# '
Line 55:1: W191 indentation contains tabs
Line 55:4: E101 indentation contains mixed spaces and tabs
Line 55:7: E128 continuation line under-indented for visual indent
Line 56:1: W191 indentation contains tabs
Line 56:4: E101 indentation contains mixed spaces and tabs
Line 56:7: E128 continuation line under-indented for visual indent
Line 57:1: W191 indentation contains tabs
Line 57:4: E101 indentation contains mixed spaces and tabs
Line 57:7: E128 continuation line under-indented for visual indent

Line 114:9: E731 do not assign a lambda expression, use a def
Line 117:14: E127 continuation line over-indented for visual indent

Line 11:1: E302 expected 2 blank lines, found 1
Line 13:80: E501 line too long (96 > 79 characters)
Line 14:80: E501 line too long (96 > 79 characters)
Line 16:80: E501 line too long (92 > 79 characters)
Line 38:80: E501 line too long (82 > 79 characters)
Line 41:80: E501 line too long (84 > 79 characters)
Line 44:80: E501 line too long (82 > 79 characters)
Line 45:80: E501 line too long (86 > 79 characters)
Line 51:80: E501 line too long (80 > 79 characters)
Line 53:80: E501 line too long (85 > 79 characters)
Line 63:80: E501 line too long (90 > 79 characters)
Line 65:80: E501 line too long (90 > 79 characters)
Line 66:80: E501 line too long (81 > 79 characters)
Line 68:80: E501 line too long (85 > 79 characters)
Line 85:80: E501 line too long (87 > 79 characters)
Line 92:80: E501 line too long (87 > 79 characters)
Line 97:80: E501 line too long (87 > 79 characters)
Line 103:80: E501 line too long (100 > 79 characters)
Line 104:80: E501 line too long (96 > 79 characters)
Line 106:80: E501 line too long (87 > 79 characters)
Line 123:80: E501 line too long (98 > 79 characters)
Line 148:80: E501 line too long (91 > 79 characters)
Line 150:80: E501 line too long (80 > 79 characters)
Line 151:80: E501 line too long (89 > 79 characters)
Line 154:80: E501 line too long (89 > 79 characters)
Line 215:80: E501 line too long (89 > 79 characters)
Line 218:80: E501 line too long (97 > 79 characters)
Line 220:80: E501 line too long (97 > 79 characters)
Line 233:40: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Line 247:80: E501 line too long (105 > 79 characters)
Line 301:80: E501 line too long (87 > 79 characters)
Line 302:80: E501 line too long (96 > 79 characters)
Line 324:80: E501 line too long (102 > 79 characters)

Line 8:1: E302 expected 2 blank lines, found 1
Line 11:1: W293 blank line contains whitespace
Line 13:1: W293 blank line contains whitespace
Line 22:73: W291 trailing whitespace
Line 25:1: W293 blank line contains whitespace
Line 41:23: E261 at least two spaces before inline comment
Line 55:25: E261 at least two spaces before inline comment
Line 58:1: W293 blank line contains whitespace
Line 62:1: W391 blank line at end of file

Line 156:80: E501 line too long (80 > 79 characters)

Line 36:5: E741 ambiguous variable name 'l'
Line 119:17: E741 ambiguous variable name 'l'
Line 201:5: E101 indentation contains mixed spaces and tabs
Line 201:5: W191 indentation contains tabs

Line 153:1: W391 blank line at end of file

Line 161:18: E741 ambiguous variable name 'l'
Line 245:38: W605 invalid escape sequence '\S'
Line 260:42: W605 invalid escape sequence '\S'

Line 116:9: E731 do not assign a lambda expression, use a def
Line 149:21: E128 continuation line under-indented for visual indent
Line 250:80: E501 line too long (81 > 79 characters)

Line 128:28: E128 continuation line under-indented for visual indent
Line 130:49: E128 continuation line under-indented for visual indent
Line 140:14: E127 continuation line over-indented for visual indent
Line 142:14: E127 continuation line over-indented for visual indent

Line 126:18: E741 ambiguous variable name 'l'
Line 129:18: E741 ambiguous variable name 'l'
Line 369:9: E741 ambiguous variable name 'I'

Line 160:5: E741 ambiguous variable name 'I'
Line 302:29: E127 continuation line over-indented for visual indent
Line 303:29: E127 continuation line over-indented for visual indent

Line 109:9: E741 ambiguous variable name 'I'
Line 190:9: E741 ambiguous variable name 'I'
Line 203:80: E501 line too long (80 > 79 characters)
Line 255:9: E741 ambiguous variable name 'I'
Line 389:9: E741 ambiguous variable name 'I'

Line 19:80: E501 line too long (80 > 79 characters)

Line 24:80: E501 line too long (80 > 79 characters)

Line 24:80: E501 line too long (80 > 79 characters)

Comment last updated at 2022-07-11 12:58:51 UTC

@mmcky
Copy link
Contributor

mmcky commented Feb 14, 2022

Thanks @Smit-create this is good practice and a good idea.

@coveralls
Copy link

coveralls commented Feb 14, 2022

Coverage Status

Coverage decreased (-1.3%) to 94.051% when pulling a88af65 on Smit-create:issue_350_1 into 0e21854 on QuantEcon:master.

@jstac
Copy link
Contributor

jstac commented Feb 23, 2022

Many thanks for working on this @Smit-create . I'll leave the review to you @mmcky . Thanks in advance.

@mmcky
Copy link
Contributor

mmcky commented Mar 23, 2022

thanks @Smit-create once we resolve the conflict I am happy for this to be merged. Thanks for putting it together.

I think this is a good idea.

Does this apply to any other modules?

@Smit-create
Copy link
Member Author

Does this apply to any other modules?

Yeah, this applies to every private module that should not be public.

@mmcky
Copy link
Contributor

mmcky commented Mar 23, 2022

Does this apply to any other modules?

Yeah, this applies to every private module that should not be public.

Perhaps we should update them all in the branch then and rename it to resolve #350

@mmcky mmcky changed the title MAINT: [Demo] Distinguish between private and public namespaces MAINT: Distinguish between private and public namespaces Mar 23, 2022
@mmcky
Copy link
Contributor

mmcky commented Mar 24, 2022

@Smit-create looks like the one failure on os-x is due to a small tolerance issue. I will re-run the workflow to check if it is a stochastic error (as the exact same test didn't fail on conda-build.

Is there any reason we can't migrate to just conda testing now?

@Smit-create
Copy link
Member Author

Is there any reason we can't migrate to just conda testing now?

Yeah, we can move to conda testing. But I'll suggest first moving all the tests to pytest and then moving to conda build.

@Smit-create
Copy link
Member Author

if it is a stochastic error (as the exact same test didn't fail on conda-build.

It might be some platform+conda specific tolerance issue. Maybe we can slightly raise the tolerance value to make it pass.

@mmcky
Copy link
Contributor

mmcky commented May 9, 2022

thanks for working on this @Smit-create. The diff makes it a bit tricky to track the changes -- for some reason the files aren't showing as modified (post a rename) but deleted and new files in place. Is this due to a force push? We will have to lean on the tests in that case. Can you ping me once the test suite is passing. Thanks.

@Smit-create
Copy link
Member Author

The diff makes it a bit tricky to track the changes -- for some reason the files aren't showing as modified (post a rename) but deleted and new files in place

New files will have an underscore before their names, while the other files will be shown as modified.

We will have to lean on the tests in that case.

Yeah, that's true. The diff is looking lengthy to review, but it all just follows the pattern which is followed in OP.

@mmcky
Copy link
Contributor

mmcky commented May 10, 2022

New files will have an underscore before their names, while the other files will be shown as modified.

Hmm. On other PR;s I have seen renamed files (with diffs). I'll have to try and find one, maybe it's because I tend to use git mv etc. but not worries. We can rely on tests when they are passing.

@Smit-create Smit-create force-pushed the issue_350_1 branch 2 times, most recently from 87ff9d4 to 0961efd Compare May 10, 2022 05:47
@Smit-create
Copy link
Member Author

I tend to use git mv

I also have used the same. But the reason is, after renaming, I am creating a new file of the same name say:

  1. I rename ecdf to _ecdf
  2. I create a new file with the name ecdf and add depreciation warnings here to avoid breaking the existing code for our current users.

@mmcky
Copy link
Contributor

mmcky commented May 10, 2022

I create a new file with the name ecdf and add depreciation warnings here to avoid breaking the existing code for our current users.

Ah ha -- that make sense! Thanks @Smit-create

@mmcky
Copy link
Contributor

mmcky commented May 11, 2022

@jstac I will review this PR against our lecture sites. But just wanted to tag you re: updates to interface in the quantecon package. @Smit-create has done a great job at adding deprecation notices for the import changes -- so no current code out there that uses quantecon should break with this update.

The effect of this will be to make .<tab> in jupyter notebooks MUCH cleaner.

@mmcky
Copy link
Contributor

mmcky commented Nov 22, 2022

@Smit-create sorry it has taken me so long to loop around to this. Do you mind updating to the latest master and then I'll merge?

@mmcky mmcky added this to the v0.6.0 milestone Nov 22, 2022
@Smit-create Smit-create force-pushed the issue_350_1 branch 2 times, most recently from e94f975 to 0ab7c73 Compare November 24, 2022 06:20
@mmcky
Copy link
Contributor

mmcky commented Dec 17, 2022

These changes applied, how will the documentation look?

@oyamad all public facing (importable) classes and functions should still all show up in the documentation. There may be some helper functions that won't be visible. Once this is updated I will take a closer look and see if we can do a test build of rtd

@Smit-create
Copy link
Member Author

thanks @Smit-create I think I have all other v0.6.0 changes merged in if you have time to loop through this again to rebase on latest master

I have rebased this branch on the lastest master branch. Thanks @mmcky

@oyamad
Copy link
Member

oyamad commented Dec 17, 2022

How will the development procedure go given this?
Suppose that a contributer wanted to implement something in a new file. Then they are supposed to write the code in _xxx.py and also create xxx.py in which they write from . import _xxx? (An instruction written somewhere would be helpful!)

@oyamad
Copy link
Member

oyamad commented Dec 17, 2022

@Smit-create This is not something caused by this PR, but would you fix these warnings (showing up during a documentation build) (change Note to Notes)?

/Applications/anaconda3/lib/python3.10/site-packages/numpydoc/docscrape.py:460: UserWarning: Unknown section Note in the docstring of rouwenhorst in /Applications/anaconda3/lib/python3.10/site-packages/quantecon/markov/approximation.py.
  warn(msg)
/Applications/anaconda3/lib/python3.10/site-packages/numpydoc/docscrape.py:460: UserWarning: Unknown section Note in the docstring of tauchen in /Applications/anaconda3/lib/python3.10/site-packages/quantecon/markov/approximation.py.
  warn(msg)

@Smit-create
Copy link
Member Author

Suppose that a contributer wanted to implement something in a new file. Then they are supposed to write the code in _xxx.py and also create xxx.py in which they write from . import _xxx? (An instruction written somewhere would be helpful!)

Suppose someone needs to write a new routine ABC which needs to be imported from quantecon directly, then he should just create a new file say _xxx.py, and in quantecon.__init__ import it as from ._xxx import ABC.

The current PR creates a new file xxx.py and adds some warnings in order to support backward compatibility. We can safely remove the files with xxx.py after maybe a year or two. This way we have a clear picture of what needs to be in quantecon's namespace and what needs to be in quantecon.some_sub_module. You can have a look at this #601 (comment) for a more clear explanation.

@Smit-create
Copy link
Member Author

but would you fix these warnings (showing up a documentation build) (change Note to Notes)?

Thanks, fixed that.

Docs build log

% make html   
python qe_apidoc.py
sphinx-build -b html -d build/doctrees   source build/html
Running Sphinx v5.3.0
OMP: Info #273: omp_set_nested routine deprecated, please use omp_set_max_active_levels instead.
making output directory... done
WARNING: html_static_path entry '_static' does not exist
WARNING: Support for emitting HTML 4 output is deprecated and will be removed in Sphinx 7. ("html4_writer=True detected in configuration options)
loading intersphinx inventory from http://docs.python.org/objects.inv...
intersphinx inventory has moved: http://docs.python.org/objects.inv -> https://docs.python.org/3/objects.inv
[autosummary] generating autosummary for: game_theory.rst, game_theory/brd.rst, game_theory/fictplay.rst, game_theory/game_generators/bimatrix_generators.rst, game_theory/lemke_howson.rst, game_theory/localint.rst, game_theory/logitdyn.rst, game_theory/mclennan_tourky.rst, game_theory/normal_form_game.rst, game_theory/pure_nash.rst, ..., tools/rank_nullspace.rst, tools/robustlq.rst, util.rst, util/array.rst, util/combinatorics.rst, util/common_messages.rst, util/notebooks.rst, util/numba.rst, util/random.rst, util/timing.rst
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 65 source files that are out of date
updating environment: [new config] 65 added, 0 changed, 0 removed
reading sources... [100%] util/timing                                          
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] util/timing                                           
generating indices... genindex py-modindex done
highlighting module code... [ 43%] quantecon.game_theory.game_generators.bimatrihighlighting module code... [100%] quantecon.util.timing                       
writing additional pages... search done
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 2 warnings.

The HTML pages are in build/html.

Build finished. The HTML pages are in build/html.

@mmcky
Copy link
Contributor

mmcky commented Dec 17, 2022

It looks like rtd will not build a version for non-local branches.

@oyamad I built this locally with the results
html.zip

The top level listing looks pretty similar with no major changes.

# Imports that should be deprecated with markov package
from .markov import mc_compute_stationary, mc_sample_path

# Imports that are deprecated and will be removed in further versions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Smit-create do we have any infrastructure that issues a Warning for users that do import these that advises them to move towards the new import path (in version 0.7.0) etc.?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to hold up the release but if we don't then perhaps we can add this to help with migration given these are still available at the top level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works in the following way:

>>> from quantecon.ecdf import ECDF  # works but throws a warning
<stdin>:1: DeprecationWarning: Please use `ECDF` from the `quantecon` namespace, the `quantecon.ecdf` namespace is deprecated. You can use the following instead:
 `from quantecon import ECDF`.
>>> from quantecon import ECDF

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Smit-create I thought that was the case. Once we identify a deprecation point I might update these in a future release to let users know when that will happen.

@mmcky
Copy link
Contributor

mmcky commented Dec 17, 2022

How will the development procedure go given this?
Suppose that a contributer wanted to implement something in a new file. Then they are supposed to write the code in _xxx.py and also create xxx.py in which they write from . import _xxx? (An instruction written somewhere would be helpful!)

Just saw this @oyamad -- looping round re: contribution / style guide is a great idea. I'll open an issue to track this. #665

@mmcky
Copy link
Contributor

mmcky commented Dec 17, 2022

@oyamad are you happy for me to merge this to master. I will then loop through our lectures

  • python-programming
  • python
  • python-advanced

to check that the main branch is working across our projects before cutting a release (likely tomorrow or Monday).

It's a high volume change -- so I want to make sure you're OK with this :-).

@mmcky mmcky self-requested a review December 17, 2022 04:50
@mmcky
Copy link
Contributor

mmcky commented Dec 17, 2022

thanks @Smit-create for all your work on this. It's been a long time coming but I will merge this now and do some final testing of the release candidate on master

@mmcky mmcky merged commit b0ac7e8 into QuantEcon:master Dec 17, 2022
@Smit-create Smit-create deleted the issue_350_1 branch December 17, 2022 04:53
@mmcky mmcky mentioned this pull request Dec 18, 2022
Smit-create added a commit to Smit-create/QuantEcon.py that referenced this pull request Dec 20, 2022
Smit-create added a commit to Smit-create/QuantEcon.py that referenced this pull request Dec 22, 2022
Smit-create added a commit to Smit-create/QuantEcon.py that referenced this pull request Dec 22, 2022
Smit-create added a commit that referenced this pull request Dec 31, 2022
Revert "MAINT: Distinguish between private and public namespaces (#601)"
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.

6 participants