dwave-networkx icon indicating copy to clipboard operation
dwave-networkx copied to clipboard

Convert to namespace package

Open arcondello opened this issue 5 years ago • 7 comments

Makes use of the new dwave.plugins namespace introduced with the release of dwave-qiskit-plugin.

The old namespaces still work, but now raise a deprecation warning.

Todo: figure out what to do with the tests (which still import from the old namespace), more testing

arcondello avatar Dec 09 '20 21:12 arcondello

For tests, there are three obvious options:

  1. Continue testing using the old namespace under the assumption that it covers both
  2. Switch to testing the new namespace and just test that the wrapper works
  3. Test both, probably with parameterized or similar

I am leaning towards (2). Thoughts?

arcondello avatar Dec 10 '20 16:12 arcondello

@arcondello, I like option (3). Seems simplest (with parameterized) / no new tests needed, covers everything, and can be easily removed when we kill the old ns.

randomir avatar Dec 15 '20 20:12 randomir

I am not sure I would describe it as easily removed, since you end up doing something like

import dwave.plugins.networkx as dnx
import dwave_networkx as old_dnx

@parameterized_class('dnx', [dnx, old_dnx])
class TestThingy(unittest.TestCase):
    def test_a(self):
        G = self.dnx.chimera_graph(4)

so removing means changing a lot of self.dnx to dnx. Not terrible but... I'll see if I can think of a nicer way.

arcondello avatar Dec 18 '20 19:12 arcondello

Also, we have quite a few tests, test files, and import statements like

from dwave_networkx.algorithms.canonicalization import rooted_tile

not just a simple import dwave.plugins.networkx as dnx.

And given python's import caching (and our namespace clone magic), I'm not 100% sure dnx and old_dnx wouldn't point to the same object.

Given all that, (2) might actually be easier.

randomir avatar Dec 18 '20 20:12 randomir

Yeah, I am actually already halfway through refactoring a lot of that as a separate PR, because we're not really consistent. Worth doing in either case.

arcondello avatar Dec 18 '20 20:12 arcondello

Pr mentioned above: https://github.com/dwavesystems/dwave-networkx/pull/186

arcondello avatar Dec 18 '20 21:12 arcondello

Codecov Report

Merging #185 (e2180c6) into main (ab75abb) will increase coverage by 0.37%. The diff coverage is 98.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   71.23%   71.60%   +0.37%     
==========================================
  Files          26       27       +1     
  Lines        1585     1606      +21     
==========================================
+ Hits         1129     1150      +21     
  Misses        456      456              
Impacted Files Coverage Δ
...plugins/networkx/drawing/distinguishable_colors.py 28.57% <ø> (ø)
dwave/plugins/networkx/generators/markov.py 86.36% <ø> (ø)
dwave/plugins/networkx/package_info.py 100.00% <ø> (ø)
dwave/plugins/networkx/drawing/pegasus_layout.py 17.91% <75.00%> (ø)
dwave/plugins/networkx/algorithms/__init__.py 100.00% <100.00%> (ø)
...ve/plugins/networkx/algorithms/canonicalization.py 100.00% <100.00%> (ø)
dwave/plugins/networkx/algorithms/clique.py 95.00% <100.00%> (ø)
dwave/plugins/networkx/algorithms/coloring.py 96.47% <100.00%> (ø)
dwave/plugins/networkx/algorithms/cover.py 100.00% <100.00%> (ø)
...lugins/networkx/algorithms/elimination_ordering.py 86.94% <100.00%> (ø)
... and 20 more

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 ab75abb...e2180c6. Read the comment docs.

codecov-io avatar Dec 21 '20 17:12 codecov-io