reduce pollution of pg namespace by pijyoi · Pull Request #1846 · pyqtgraph/pyqtgraph

pijyoi · 2021-06-20T06:18:36Z

This PR adds __all__ to the various files imported into pg namespace by __init__.py.

functions.py stands out in particular as it defines many functions, some of which are really only meant for use within the library. For backwards compatibility, the functions are still exported except for those functions which have only been added recently. The preferred way of accessing those functions is via pg.functions.

Most functions in colormap.py also look like they shouldn't reside within pg namespace. @NilsNemitz, could you provide some input here?

Potentially this PR could break user code that used accidental imports from pg namespace. E.g.

python -c "import pyqtgraph as pg; print(pg.warnings)"

NilsNemitz · 2021-06-20T07:58:23Z

pyqtgraph/colormap.py

@@ -1,3 +1,7 @@
+__all__ = [
+    # 'listMaps', 'get',
+    'getFromMatplotlib', 'getFromColorcet', 'makeMonochrome', 'modulatedBarData', 'ColorMap']


I think
__all__ = ['ColorMap']
should be sufficient here. The function names were chosen to live in a separate colormap namespace and don't make much sense as e.g. pg.getFromColorcet (or even pg.get)

To me it seems cleaner to have
pg.colormap.get('CET1')
rather than
pg.getColormap('CET1')
and it doesn't require any more typing either.

For the stuff in functions.py we should maybe start by changing the documentation, and conceptionally move the more abstract functions to live in pg.functions.

My personal preference for deciding what should live in the top-level pg. namespace would be to ask "Would I need this when doing manual data visualization in an interactive notebook?" That is where short, easily memorized calls are most relevant. From that point of view, the pg. namespace should hold things like mkColor(), but not arrayToQPath().

As far as I can tell, the use of the more abstract functions throughout the library is typically something like

import pg.functions as fn fn.eq(a,b)

and maybe the documentation should encourage that use, too.

Right now, we also have functionality like affineSliceCoords in the pg. namespace, even though this is not even in the documentation.

This is obviously a policy discussion that requires more input from more people, but from the technical side of things:
Would there be a convenient way to add deprecation messages for the use of selected abstract functions directly from the pg. namespace (while keeping the access through pg.functions indisturbed)?

I have implemented the deprecation following the pointer to https://www.python.org/dev/peps/pep-0562/ from @ntjess

NilsNemitz · 2021-06-20T08:35:09Z

examples/verlet_chain/chain.py

-    stepped = pg.QtCore.Signal()
-    relaxed = pg.QtCore.Signal()
+    stepped = QtCore.Signal()
+    relaxed = QtCore.Signal()


I have no actual objection to this change, but having this as
pg.QtCore might be a useful reminder that this version of QtCore has been touched by the import into PyQtGraph.
In this specific case, the code relies on the patched-in alias QtCore.Signal = QtCore.pyqtSignal when running on PyQt.

Where that isn't the case and only the normal functionality is used, simplifying pg.QtCore to QtCore seems like a good idea.

I have created fake QtCore, QtGui, QtWidgets so that pyqtgraph's monkeying of the module attributes are local.
In particular, QtCore.Signal = QtCore.pyqtSignal is now a local change.

Monkey-patching in methods to classes will still modify the actual classes, but most of those are deprecated and slated for removal in 0.13.0

pijyoi · 2021-06-20T08:40:46Z

Technically, QtCore was never directly imported into pg namespace, only indirectly by "import *".
I have changed all in-library uses of pg.QtCore to pg.Qt.QtCore, and there were quite a number of them.
Given that users are likely to have gained a dependency on pg.QtCore, I have explicitly imported QtCore into pg namespace. Unless anyone thinks otherwise?

Personally, I don't use pg.QtGui not pg.QtCore, and my code starts like this:

import pyqtgraph as pg
from pyqtgraph.Qt import QtCore, QtGui, QtWidgets

or

from PySide6 import QtCore, QtGui, QtWidgets
import pyqtgraph as pg

NilsNemitz · 2021-06-20T08:49:49Z

tests/widgets/test_graphics_view.py

    # XXX: Removal of central item is not clear in code
    scene = view.sceneObj
-    assert isinstance(scene, pg.GraphicsScene)
+    assert isinstance(scene, pg.GraphicsScene.GraphicsScene)


Do we have a clear convention for the capitalization of modules?
This one probably does not need to be accessed by the user typically, and thus doesn't justify a change.

But in an ideal world (with no existing user code), should this change to pg.graphicsscene.GraphicsScene?
I would argue that a no-caps convention is used by e.g. pg.functions and pg.colormap.

Well, whatever convention there might be, it can't be retroactively applied to modules that have been around such a long time.

I'll give you that, although this may be the first time that we are encouraging access outside of the pg. namespace. :)

I should have really phrased that more openly, though. Sorry about that!

Is there anything we can do to keep module names as consistent as possible in the future?
Do you have a preference for the naming?
I do like no-caps for modules, and find e.g. pg.GraphicsScene.GraphicsScene a little confusing. But if it turns out that we really like CamelCase names for the modules, then we might still be able to change the module name e.g. for colormap.py. All the module level functions are still marked experimental there, but if we wanted to rename it, we probably shouldn't wait any longer.

This shouldn't be any holdup for this PR, though.

although this may be the first time that we are encouraging access outside of the pg. namespace.

That's a question to be answered for GraphicsScene. It was imported into pg namespace only indirectly. Only one place in the library accessed it as pg.GraphicsScene. So I had a choice of either importing it explicitly into pg namespace or modifying the access to pg.GraphicsScene.GraphicsScene. I picked the latter, but if anyone feels that the former would be a better choice, I can make the change.

Do you have a preference for the naming?

Well, I prefer snake_case. :-) But there's a rule that when modifying code, try to make it look like the rest.

@pijyoi regarding snake case wonder if we should try to integrate the from __feature__ import snake_case Not sure what's involved with that level of integration though.

ntjess · 2021-06-20T23:12:04Z

Potentially this PR could break user code that used accidental imports from pg namespace. E.g.
python -c "import pyqtgraph as pg; print(pg.warnings)"

FWIW, __getattr__ can be defined on the module to throw a deprecation warning for those cases, right?

https://stackoverflow.com/a/48916205/9463643

In Python 3.7+, you just use the one obvious way. To customize attribute access on a module, define a __getattr__ function at the module level which should accept one argument (name of attribute), and return the computed value or raise an AttributeError:
def __getattr__(name: str) -> Any:
  # ...
This will also allow hooks into "from" imports, i.e. you can return dynamically generated objects for statements such as from my_module import whatever.

pijyoi · 2021-06-21T01:52:08Z

FWIW, getattr can be defined on the module to throw a deprecation warning for those cases, right?

Thanks, I have used it to deprecate access of functions in functions.py via pg.

The example I used pg.warnings was a bad one. If the user accesses Python library imports like that, that's on the user.

pijyoi · 2021-06-21T13:22:41Z

Potentially, CIELabColor, colorCIELab and colorDistance could be removed from the pg. namespace since they were only very recently added to the functions.rst documentation, so we could still remove them from the docs.

NilsNemitz · 2021-06-21T13:49:41Z

I would say that the basic color functions live quite nicely in the pg. namespace, and that the most logical place to add CIELabColor and colorCIELab would be alongside HSVColor and colorTuple... even if they are probably not used quite as much.

I wouldn't mind moving colorDistance to functions... But it would be nice to have a strategy of what goes where after we are done moving things around. Even colorDistance seems much more user facing to me than e.g. arrayToQPath.

pijyoi · 2021-06-22T05:00:18Z

These are the attributes from functions.py marked as deprecated if accessed from pg. namespace.
They are not documented in functions.rst

In [1]: import pyqtgraph as pg

In [2]: print(pg.functions.deprecated_import_all)
['disconnect', 'makeArrowPath', 'traceImage', 'colorToAlpha', 'downsample', 'siApply', 'interpolateArray',
 'toposort', 'SignalBlock', 'invertQTransform', 'interweaveArrays', 'affineSliceCoords', 'Color', 'subArray']

There aren't that many of them, so one other safe option is to not deprecate them and henceforth be more judicious about what new functions qualify for going into __all__

attribute	only used-by
makeArrowPath	ArrowItem.py
traceImage	nobody
colorToAlpha	nobody
downsample	ImageItem.py
siApply	siEval, SpinBox.py
interpolateArray, affineSliceCoords	affineSlice
toposort	Flowchart.py
subArray	nobody

pijyoi · 2021-06-23T02:19:00Z

I have removed policy-related decisions from the changes. So, no deprecations. If any indirectly-imported attribute was used within the library, then it gets explicitly imported into pg namespace.

Exceptions:

pg.np
pg.QT_LIB (only used within test_matplotlib.py to skip for Qt6. should be removed soon)
very recently added helper functions to functions.py (shown as commented out within all list)

j9ac9k · 2021-06-23T02:30:55Z

Sorry I've been busy and haven't had time to chime in here. I'm fully supportive of the effort to minimize the modules, classes and functions in the pg namespace.

Feel free to be aggressive about moving things around using deprecation warnings with pointing to where the moved objects can be accessed from.

FYI, MetaArray will be getting removed from the library in a few versions.

j9ac9k · 2021-06-23T05:39:26Z

@pijyoi what you did with the "fake" Qt modules to avoid monkey patching them is brilliant.

I'm good w/ merging this; will give some time for others to chime in.

NilsNemitz · 2021-06-23T06:05:25Z

I don't have the time to contribute a well-argued reply, but I wanted to say that I like this effort a lot.

I personally believe that this should be a continued after this PR; The next step might be to change the documentation to encourage using the more technical utility functions from the pg.functions namespace, which most of the library code already does.

ntjess · 2021-06-23T15:07:18Z

What are your thoughts on exposing parametertree in the pg namespace as well? I.e. supporting tree = pg.ParameterTree. The imports are well-defined in parametertree/__init__.py

pijyoi · 2021-06-23T15:50:55Z

The comments in __init__.py say:

## don't import the more complex systems--canvas, parametertree, flowchart, dockarea
## these must be imported separately.

So leaving them out was a deliberate choice, not an accidental oversight.

j9ac9k · 2021-06-23T18:49:59Z

@pijyoi sorry you haven't been able to get more direct input here. This is the sort of thing my brain defaults to "its just how it is" and I go with it; so I'm likely go to along with most proposed changes.

Generally speaking, I am very supportive of cleaning up the namespace. I'm very supportive of putting deprecation warnings on items currently in the namespace but there is general agreement that it should not be there, and getting the ball rolling on moving them out.

In another PR/issue, we discussed the use-case of camelCase vs. snake_case being a reflection of whether the method/function was intended to be public or not. I think you were onto something there; but I do think private-ish, should start with an _, which we currently do (mostly).

I do wonder if we can tap into PySide6's from __feature__ import snake_case and effectively support both methods (the later of which is only accessible, I'm not sure if it's feasible we are able to use that or not.

I do think some of the colormap stuff is a bit confusing (colormap module, ColorMap class) ....but that's likely another PR.

I think this PR is good as is.

For the more potentially controversial cases, perhaps I should start an issue (or a discussion?) that we can discuss more specific stuff there?

ntjess · 2021-06-23T19:04:40Z

@j9ac9k Nim ftw :)

https://nimacademy.com/docs/basics/#assigning-variables

pijyoi · 2021-06-24T01:22:29Z

UPDATE: For the record, these are the symbols eliminated from the pg namespace.

Out[7]:
{'Colors',
 'FLOAT_REGEX',
 'INT_REGEX',
 'ImageViewTemplate_pyside6',
 'IsosurfaceDataCache',
 'LRU',
 'MATRIX_RGB_FROM_XYZ',
 'MATRIX_XYZ_FROM_RGB',
 'QT_LIB',
 'QtVersion',
 'SI_PREFIXES',
 'SI_PREFIXES_ASCII',
 'SI_PREFIX_EXPONENTS',
 'Symbols',
 'VECTOR_XYZn',
 'ViewBoxMenu',
 'arrayToQPolygonF',
 'atan2',
 'axisCtrlTemplate_pyside6',
 'basestring',
 'create_qpolygonf',
 'ctypes',
 'dataType',
 'decimal',
 'degrees',
 'division',
 'fn',
 'getNumbaFunctions',
 'hypot',
 'isSequence',
 'makeCrosshair',
 'math',
 'ndarray_from_qpolygonf',
 'ndarray_to_qimage',
 'np',
 'operator',
 'plotConfigTemplate_pyside6',
 'qimage_to_ndarray',
 're',
 'reduce',
 'string',
 'struct',
 'try_fastpath_argb',
 'warnings',
 'weakref'}

j9ac9k · 2021-06-24T01:34:04Z

MetaArray might be an issue to remove. @outofculture how she's Acq4 import that module?

pijyoi · 2021-06-24T02:09:29Z

Well, it wasn't so much removed as it was not added in. If it was in pg namespace, it was an indirect import due to lack of __all__. None of the examples makes use of it by pg.MetaArray.

outofculture · 2021-06-24T07:54:20Z

MetaArray might be an issue to remove. @outofculture how she's Acq4 import that module?

ACQ4 uses from pyqtgraph, from pyqtgraph.metaarray and pg.MetaArray, so, yeah. ACQ4 also uses OrderedDict, which is deprecated and will get removed in v0.13. I think for both of these, the better course is to deprecate them and remove them, but leave them in place until then.

pijyoi · 2021-06-24T08:03:25Z

ACQ4 also uses OrderedDict

Just to clarify, did you mean that ACQ4 accesses it as pg.OrderedDict?

outofculture · 2021-06-24T08:04:03Z

ACQ4 also uses OrderedDict

Just to clarify, did you mean that ACQ4 accesses it as pg.OrderedDict?

Yes, correct.

outofculture · 2021-06-24T08:31:03Z

pyqtgraph/__init__.py

+
+# indirect imports known to be used outside of the library
+from .metaarray import MetaArray
+from collections import OrderedDict


This import line will obscure the deprecation warning for anyone importing OrderedDict from pyqtgraph. We should keep it as from .orderedict so as to properly prepare such importers.

I didn't think of that, so I tried it out. It seems that from collections yields the same behavior as current master whereas from .ordereddict obscures it.

current master

d:\>py -c "import pyqtgraph as pg; pg.OrderedDict" d:\>py -c "from pyqtgraph.ordereddict import OrderedDict" <string>:1: DeprecationWarning: OrderedDict is in the standard library for supported versions of Python. Will be removed in 0.13

with from collections import OrderedDict

d:\>py -c "import pyqtgraph as pg; pg.OrderedDict" d:\>py -c "from pyqtgraph.ordereddict import OrderedDict" <string>:1: DeprecationWarning: OrderedDict is in the standard library for supported versions of Python. Will be removed in 0.13

with from .ordereddict import OrderedDict

d:\>py -c "import pyqtgraph as pg; pg.OrderedDict" d:\>py -c "from pyqtgraph.ordereddict import OrderedDict"

Yeah, I realized that would be the case after I went to bed last night. I guess the better warning here would be to use from .ordereddict in __init__.py and replace ordereddict.py with:

from collections import OrderedDict as _OrderedDict import warnings class OrderedDict(_OrderedDict): def __init__(self, *a, **kw): warnings.warn( "OrderedDict is in the standard library for supported versions of Python. Will be removed in 0.13", DeprecationWarning, stacklevel=2, ) super(OrderedDict, self).__init__(*a, **kw)

I'm 👍 for warnings in __init__ method (it's what I did for MetaArray)

j9ac9k · 2021-06-24T22:20:05Z

I think this is ready to merge, @outofculture @ixjlyons ?

ixjlyons · 2021-06-25T05:48:59Z

Looks reasonable to me. The local Qt modules thing is a neat trick, though I admit I was a little puzzled by it until I read more context here in the comments. A comment about localizing the effects of the monkey-patching might be nice.

Funny enough, it does add another item to the top-level pyqtgraph namespace (imports), but I guess that's not really a big deal. That could probably be avoided by pushing Qt.py down into a package:

pyqtgraph/
    Qt/
        imports/
            QtCore/
                __init__.py
            QtGui/
                __init__.py
            QtWidgets/
                __init__.py
        __init__.py  # current contents of pyqtgraph/Qt.py

Maybe not worth it, just a thought.

pijyoi · 2021-06-26T00:03:42Z

I have changed the directory hierarchy to the following.

pyqtgraph/
    Qt/
        QtCore/
            __init__.py
        QtGui/
            __init__.py
        QtWidgets/
            __init__.py
    __init__.py  # contents of pyqtgraph/Qt.py

j9ac9k · 2021-06-26T06:26:10Z

Love the change, nice catch @ixjlyons thanks for implementing @pijyoi

I think this is ready to merge. Thanks for making this happen @pijyoi !

NilsNemitz reviewed Jun 20, 2021

View reviewed changes

pijyoi force-pushed the import_hygiene branch from 43f1c2a to a775d09 Compare June 23, 2021 02:09

pijyoi marked this pull request as ready for review June 23, 2021 02:19

j9ac9k mentioned this pull request Jun 24, 2021

Prep for 0.12.2 release #1838

Merged

5 tasks

outofculture reviewed Jun 24, 2021

View reviewed changes

pijyoi force-pushed the import_hygiene branch from 092510b to 0bd633e Compare June 24, 2021 20:14

pijyoi added 3 commits June 25, 2021 21:19

add __all__ to various files

81baa18

add imports used by ACQ4

427128e

deprecate ordereddict.OrderedDict on instantiation

11bf824

pijyoi force-pushed the import_hygiene branch from a95a06c to f56a67e Compare June 25, 2021 13:23

move Qt.py to Qt/__init__.py

3a1b74d

pijyoi force-pushed the import_hygiene branch from f56a67e to 3a1b74d Compare June 25, 2021 13:35

remove imports subdir

5d55f3f

j9ac9k merged commit 93fa866 into pyqtgraph:master Jun 26, 2021

pijyoi deleted the import_hygiene branch June 26, 2021 12:24

j9ac9k mentioned this pull request Jul 2, 2021

bug: PyQt6 does not have localPos() #1875

Merged

j9ac9k mentioned this pull request Aug 1, 2021

More import fixes for cx_freeze #1801

Closed

Uh oh!

Conversation

pijyoi commented Jun 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pijyoi commented Jun 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntjess commented Jun 20, 2021

Uh oh!

pijyoi commented Jun 21, 2021

Uh oh!

pijyoi commented Jun 21, 2021

Uh oh!

NilsNemitz commented Jun 21, 2021

Uh oh!

pijyoi commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pijyoi commented Jun 23, 2021

Uh oh!

j9ac9k commented Jun 23, 2021

Uh oh!

j9ac9k commented Jun 23, 2021

Uh oh!

NilsNemitz commented Jun 23, 2021

Uh oh!

ntjess commented Jun 23, 2021

Uh oh!

pijyoi commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j9ac9k commented Jun 23, 2021

Uh oh!

ntjess commented Jun 23, 2021

Uh oh!

pijyoi commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j9ac9k commented Jun 24, 2021

Uh oh!

pijyoi commented Jun 24, 2021

Uh oh!

outofculture commented Jun 24, 2021

Uh oh!

pijyoi commented Jun 24, 2021

Uh oh!

outofculture commented Jun 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

outofculture Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j9ac9k commented Jun 24, 2021

Uh oh!

ixjlyons commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

pijyoi commented Jun 22, 2021 •

edited

Loading

pijyoi commented Jun 23, 2021 •

edited

Loading

pijyoi commented Jun 24, 2021 •

edited

Loading

outofculture Jun 24, 2021 •

edited

Loading

ixjlyons commented Jun 25, 2021 •

edited

Loading