DM-7292: Port to Python 3 #77
Conversation
fred3m
force-pushed
the
tickets/DM-7292
branch
from
6daef27
to
9daa645
Compare
| @@ -183,9 +184,12 @@ def assertCatalogsEqual(self, catalog1, catalog2, skipCols=()): | |||
| self.assertEqual(catalog1.schema, catalog2.schema) | |||
| self.assertEqual(len(catalog1), len(catalog2)) | |||
| d = catalog1.schema.extract("*") | |||
| def fixNaN(x): | |||
| if x!=x: | |||
| return "NaN" | |||
| fixNaN = lambda x: x if x == x else "NaN" | |||
There was a problem hiding this comment.
Should this lambda be removed with the introduction of the fixNaN function?
There was a problem hiding this comment.
Yes, I think I was in the middle of making this change when something interrupted me and since it didn't break anything I must have forgotten about it. Great catch.
mrawls
force-pushed
the
tickets/DM-7292
branch
from
6dcec83
to
c6f99bd
Compare
mrawls
force-pushed
the
tickets/DM-7292
branch
2 times, most recently
from
5164654
to
0f214e6
Compare
mrawls
force-pushed
the
tickets/DM-7292
branch
from
0f214e6
to
7788d97
Compare
| simSrcRecord.setL(self.objectIdKey, truthRecord.getId()) | ||
| simSrcRecord.setL(self.exposureIdKey, obsRecord.getId()) | ||
| simSrcRecord.setI(self.objectIdKey, truthRecord.getId()) | ||
| simSrcRecord.setI(self.exposureIdKey, obsRecord.getId()) |
There was a problem hiding this comment.
Have you verified that integers can hold the values we use, and won't be too short?
There was a problem hiding this comment.
There is much confusion over L vs I in the C++ interface layer. Python (2 or 3) both use at least 64-bits for normal int type but in most cases C++ treats I as a 32-bit integer and that would indeed be too short for our 64-bit IDs.
See the discussion on RFC-227.
| self.assertAlmostEqual(bgMean, 191.51595080958367, places=7) | ||
| self.assertAlmostEqual(bgStdDev, 0.22492169148323429, places=7) | ||
| self.assertAlmostEqual(bgMean, 191.51595080958367, places=3) | ||
| self.assertAlmostEqual(bgStdDev, 0.22492169148323429, places=3) |
There was a problem hiding this comment.
7 places to 3 places is a huge change! Do differences in software stacks really produce different numbers at the 10^-(3/4) level? If so that is worrying. It implies we might have reproducibility issues in the future. Is this test able to be tightened up to ~5? What are the magnitude of differences we are seeing between versions? Is this related to changing to ints places? Are we losing precision in calculations? I know these are kind of big questions to ask, but making changes to unit tests to make things work scares me unless we can really justify why the change is nessisary.
| # The values asserted here are the precise result that py2 gives. The precision below | ||
| # should be tightened and this comment removed in the course of DM-8017. | ||
| # Also see: https://community.lsst.org/t/difference-in-py2-and-py3-background-models/1240 | ||
|
|
There was a problem hiding this comment.
I guess disregard the past comment as I got to this last. As there is a ticket to do this work, I see no need to do the work on this ticket.
In python 3 longs are not recognized, so it is necessary to use int's instead of longs and setI/getI methods.
The tests were updated in DM-6815 due to changes in the config parameters but the old tolerances were too tight. Slight differences in compilers and (apparently) python versions causes much larger variations than the old tests allow. See 3f8730a for more.
mrawls
force-pushed
the
tickets/DM-7292
branch
3 times, most recently
from
9ce9bd3
to
3fda82b
Compare
Delete "from builtins import str" rather than just comment out
mrawls
force-pushed
the
tickets/DM-7292
branch
from
0cf663e
to
13b48c0
Compare
No description provided.