DM-35681: ensure QuantumGraph is passed a DimensionUniverse at construction #265
Conversation
Codecov Report
@@ Coverage Diff @@
## main #265 +/- ##
==========================================
- Coverage 72.28% 72.17% -0.11%
==========================================
Files 60 60
Lines 6772 6779 +7
Branches 1395 1399 +4
==========================================
- Hits 4895 4893 -2
- Misses 1654 1658 +4
- Partials 223 228 +5
Continue to review full report at Codecov.
|
| @@ -173,6 +170,15 @@ def _buildGraphs( | |||
| # a newly created QuantumNode to the appropriate input/output | |||
| # field. | |||
| for quantum in quantumSet: | |||
| if quantum.dataId is not None: | |||
There was a problem hiding this comment.
I'm not really bothered by codecov's complaints here: I'm just being super defensive about trying to avoid backwards-compatibilty breakage by guessing the DimensionUniverse, but really everybody should be using the code path that is tested, in which the universe is passed explicitly (since that's what GraphBuilder).
There was a problem hiding this comment.
Just a couple questions probably more for my info than anything about the code itself. I don't know all of this code so there may be some location that was missed. But I ran the pipe_base and bps unit tests which were all fine. Also I submitted a test run on the idf that had the warning before and now it doesn't. Merge approved.
| newInst._buildGraphs( | ||
| quantumDict, | ||
| _quantumToNodeId={n.quantum: n.nodeId for n in nodes}, | ||
| _buildId=self._buildId, | ||
| metadata=self._metadata, | ||
| universe=self._universe, |
There was a problem hiding this comment.
May be perfectly correct, but seems strange to create the object with the universe value on line 631 and then pass in the same value to that object's method on line 637.
There was a problem hiding this comment.
I agree it's strange; it's an artifact of some hoops the code jumps through to avoid having the "usual" __init__ logic do a lot of unnecessary work. Basically, passing the universe to __init__ just saves __init__ from complaining that it doesn't have a universe, and then the next call actually sets up the graph (including setting the universe).
| @@ -13,7 +13,7 @@ repos: | |||
| # supported by your project here, or alternatively use | |||
| # pre-commit's default_language_version, see | |||
| # https://pre-commit.com/#top_level-default_language_version | |||
| language_version: python3.8 | |||
| language_version: python3.10 | |||
There was a problem hiding this comment.
The changes in this ticket required jumping to python 3.10?
There was a problem hiding this comment.
No, it's just something that should have been done earlier that had gone unnoticed until now (I think it only matters when someone does pre-commit install).
TallJimbo
force-pushed
the
tickets/DM-35681
branch
from
51bde6a
to
c97fc4e
Compare
This is not the Python version used to run black, and updating it to 3.10 does not break compatibility with 3.8; it just makes our formatting more future-compatible (and as of right now, in this package, it does nothing).
TallJimbo
force-pushed
the
tickets/DM-35681
branch
from
c97fc4e
to
737f6b3
Compare
Checklist
doc/changes