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

Convert to namespace package #185

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

arcondello
Copy link
Member

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
Copy link
Member Author

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?

Copy link
Member

@randomir randomir left a comment

Choose a reason for hiding this comment

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

LGTM.

@randomir
Copy link
Member

@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.

@arcondello
Copy link
Member Author

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.

@randomir
Copy link
Member

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.

@arcondello
Copy link
Member Author

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
Copy link
Member Author

Pr mentioned above: #186

@codecov-io
Copy link

codecov-io commented Dec 21, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants