Fix conditional import problem and refactor #187
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #187 +/- ##
==========================================
- Coverage 75.52% 75.42% -0.10%
==========================================
Files 34 34
Lines 5569 5551 -18
==========================================
- Hits 4206 4187 -19
- Misses 1363 1364 +1 ☔ View full report in Codecov by Sentry. |
EarlMilktea
force-pushed
the
171-bug-type-hint-is-dynamically-initialized
branch
from
2e92467
to
a233c93
Compare
| def nodes(self) -> NodesObject: | ||
| def nodes(self) -> Any: |
There was a problem hiding this comment.
We can be more precise in these type annotations: NodesObject, EdgesObject and GraphObject were not intended to be union types; they depend on whether the subclass is NXGraphState or RXGraphState. Therefore, they are type parameters of the generic class BaseGraphState, which NXGraphState and RXGraphState instantiate with the types they actually use. I propose a fix here: thierry-martinez@f2ef4b1
There was a problem hiding this comment.
Love this! Let me use your suggestion.
| def try_use_rustworkx() -> bool: | ||
| return util.find_spec("rustworkx") is not None |
There was a problem hiding this comment.
I believe it would be better to let GraphState use rustworkx by default if available, not specifically for tests but in any use cases (except for some tests such as test_pauli_measurement, where we specifically want to check that both networkx and rustworkx code paths work correctly). I propose moving rustworkx probing at GraphState level: thierry-martinez@f7e503c
There was a problem hiding this comment.
@shinich1 In the first place, I'm not sure why rustworkx was made optional. Do you remember why?
There was a problem hiding this comment.
@EarlMilktea actually that's a good point! When we first added rustworkx did not support some combinations of OS/version, thus this impl. If rustworkx is much more mature now, we can just drop networkx version. What do you think?
There was a problem hiding this comment.
It would greatly simplify the code. I'm for that.
There was a problem hiding this comment.
I also would like to do thorough refactor.
There was a problem hiding this comment.
@thierry-martinez @shinich1
I'm not opposed to keep networkx, but at the same time I'm strongly tempted to drop rxgraphviews.
This module is forcibly making rustworkx behave like networkx by translating its internal representation.
This can never be the right way.
There was a problem hiding this comment.
Additionally, let me point out that networkx has very poor type annotation support: we somehow need to isolate/wrap it to prevent Any or type warnings.
There was a problem hiding this comment.
for portability
fastflow will anyway introduce similar portability constraints as rustworkx, but it would not be a problem because for such environments even numpy is not binary-distributed in the first place.
Also see https://github.com/TeamGraphix/fastflow/blob/2-implement-flow-algorithm/.github/workflows/wheel.yml .
There was a problem hiding this comment.
being able to target multiple (two) graph back-ends help to have the right abstractions in BaseGraphState.
@thierry-martinez do you have stim integration in mind regarding above (which you mentioned are ongoing on Paris side)?
I am tempted to drop networkx, as doing so should make it easier to maintain. If stim or other stabilizer simulator may come in to this, let us keep the abstract interface. what do you think? @EarlMilktea @thierry-martinez
There was a problem hiding this comment.
I don't think that we have to keep networkx for stim, but I believe that having the networkx back-end may have several advantages: beside keeping a pure Python implementation, it can be helpful for instance to catch bugs in other backends (see #206 ).
| from rustworkx import PyGraph | ||
| else: | ||
| PyGraph = None | ||
| def try_to_networkx(g: BaseGraphState) -> nx.Graph: |
There was a problem hiding this comment.
I think the prefix try_ should be preferred for functions that gracefully return in any case, even if they failed to accomplish their goal (for instance, by returning None). This function converts g into networkx graph, and raises an exception if it is not possible; to_networkx seems to be a good name for it.
There was a problem hiding this comment.
Good point. Anyway we may need to share naming styles...
| from graphix.graphsim.rxgraphstate import RXGraphState | ||
|
|
||
| use_rustworkx = isinstance(graph, RXGraphState) | ||
| except Exception: |
There was a problem hiding this comment.
ModuleNotFoundError would be better, but I propose to let GraphState do rustworkx probing itself: thierry-martinez@f7e503c
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>
EarlMilktea
changed the title
|
Let me close this PR as |
Resolves #171 .