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
DM-32950 Make Quantum unit test deterministic #617
Conversation
tests/test_quantum.py
Outdated
@@ -68,7 +68,8 @@ def _buildFullQuantum(self, taskName, addRecords=False) -> Tuple[Quantum, Iterab | |||
|
|||
visit = universe['visit'] | |||
region = Circle() | |||
hashed = hash(visit).to_bytes(30, 'little') | |||
# create a synthetic value to mock as a visit hash | |||
hashed = hash(5786663976355850161).to_bytes(30, 'little') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to hash that big int, or just convert the big integer directly to bytes? It's not obvious to me that hashing the big integer is deterministic, but maybe you know more about its guarantees than I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I didn't mean to do that at all, thank you for pointing that out, I was in too much of a rush trying to juggle to notice my highlight replace failed me. I will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was supposed to be (5...1).to_bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, @natelust and I discussed this out-of-band, and this line isn't actually testing anything. There are two problems unrelated to this ticket that we should just punt on in order to get the main branch working again:
- passing
hash
below does nothing, becauseDimensionRecord
accepts**kwargs
and silently ignores those it doesn't recognize - it should raise instead; - the code this is trying to test, at
daf_butler/python/lsst/daf/butler/core/dimensions/_records.py
Lines 383 to 384 in ad64451
if (hsh := "hash") in rec: rec[hsh] = bytes.fromhex(rec[hsh].decode()) bytes
. But our current dimension schema only has one such field (in theskymap
dimension), and it is namedhash
.
Short version: we should just delete this line, and create some new tickets.
ee41213
to
132dbce
Compare
Codecov Report
@@ Coverage Diff @@
## main #617 +/- ##
=======================================
Coverage 83.96% 83.96%
=======================================
Files 234 234
Lines 30037 30037
Branches 4976 4976
=======================================
Hits 25222 25222
Misses 3669 3669
Partials 1146 1146
Continue to review full report at Codecov.
|
132dbce
to
0b3fdaf
Compare
tests/test_quantum.py
Outdated
@@ -68,7 +68,8 @@ def _buildFullQuantum(self, taskName, addRecords=False) -> Tuple[Quantum, Iterab | |||
|
|||
visit = universe['visit'] | |||
region = Circle() | |||
hashed = hash(visit).to_bytes(30, 'little') | |||
# create a synthetic value to mock as a visit hash | |||
hashed = hash(5786663976355850161).to_bytes(30, 'little') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, @natelust and I discussed this out-of-band, and this line isn't actually testing anything. There are two problems unrelated to this ticket that we should just punt on in order to get the main branch working again:
- passing
hash
below does nothing, becauseDimensionRecord
accepts**kwargs
and silently ignores those it doesn't recognize - it should raise instead; - the code this is trying to test, at
daf_butler/python/lsst/daf/butler/core/dimensions/_records.py
Lines 383 to 384 in ad64451
if (hsh := "hash") in rec: rec[hsh] = bytes.fromhex(rec[hsh].decode()) bytes
. But our current dimension schema only has one such field (in theskymap
dimension), and it is namedhash
.
Short version: we should just delete this line, and create some new tickets.
0b3fdaf
to
6a03f84
Compare
Checklist
doc/changes