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-23103: Update functor unittests to no longer rely on test_multilevel_parq.csv.gz #347
Conversation
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.
Looks good, except I'm a bit worried about generating fake "data" on the fly where columns are often identical to one another, and then later testing for equality/inequality. A few of the tests switch from assertFalse
to assertTrue
in this PR, and I wonder if that's because they were failing the inequality test because columns that are typically different in real data were generated to be the same.
self.dataDict["modelfit_CModel_instFlux"] = np.full(self.nRecords, 1000) | ||
self.dataDict["modelfit_CModel_instFluxErr"] = np.full(self.nRecords, 10) | ||
parq = self.simulateMultiParquet(self.dataDict) | ||
|
||
for filt in self.filters: | ||
filt = 'HSC-G' |
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.
I see this isn't your change, but while we're here, seems filt = 'HSC-G'
should be removed?
tests/test_functors.py
Outdated
df3 = self._compositeFuncVal(func2) | ||
self.assertFalse(df2.equals(df3)) | ||
df3 = self._compositeFuncVal(func2, parq) | ||
self.assertTrue(df2.equals(df3)) |
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.
This also changed from assertFalse
to assertTrue
...why? I don't remember why it was so, but should make sure that this isn't evaluating to true because of identical simulated values. This probably means that generally it would be better to make sure that simulated values in each column are different, this way equality/inequality tests will be more useful.
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.
There's a switch of the filter value just above and since the values are the same the test is now True. I've done a similar step of modifying the value for specifically R band this time force the test to fail.
Modify unittests Add lsst test wrappers Fix up parquet table creation.
Reorder toDataFrame method.
Debug testComposite functor. Debug all unittests. Fix linting Change Mag unittest behavior back. Modify composite filter test.
8ff68e9
to
62aecb5
Compare
No description provided.