Code sweep #122
Code sweep #122
Conversation
| @@ -114,7 +114,7 @@ class MultiDatabaseEntry { | |||
| std::vector<unsigned char> bytes; | |||
| }; | |||
|
|
|||
| typedef std::vector<MultiDatabaseEntry> MultiDatabaseDescriptor; | |||
| using MultiDatabaseDescriptor = std::vector<MultiDatabaseEntry>; | |||
There was a problem hiding this comment.
why did you choose to change this one typedef among all the typedefs. There's probably about a dozen other typedefs
| catalog.erase(catalog.begin() + i); | ||
| } else { | ||
| for (int i = catalog.size() - 1; i > 0; i--) { // [BUG]? catalog[catalog.size()] is invalid | ||
| if ((catalog[i].spatial - catalog[i - 1].spatial).Magnitude() < 5e-5) { // 70 stars removed at this threshold. |
There was a problem hiding this comment.
Yep, this loop indexing looks like a bug, good catch!
(Valgrind didn't find this error because the catalog is just part of the MultiDatabase -- so the first iteration of the loop read from the wrong part of the MultiDatabase, which is still memory allocated to the process).
Actually, this happens before the MultiDatabase -- the reason there isn't an error is just because of the extra empty padding at the end of the vector.
| for (int i = catalog.size() - 1; i > 0; i--) { // [BUG]? catalog[catalog.size()] is invalid | ||
| if ((catalog[i].spatial - catalog[i - 1].spatial).Magnitude() < 5e-5) { // 70 stars removed at this threshold. | ||
| if (catalog[i].magnitude > catalog[i - 1].magnitude) { | ||
| // [BUG]? If mag of i > mag of i-1, we want to keep i, not i-1 |
There was a problem hiding this comment.
you fool, brighter magnitudes have smaller CatalogStar.magnitude values! (might be worth adding a comment here to avoid confusion from future contributors)
(This behavior, for CatalogStar objects, is different than for Star objects. That's because, logically, the CatalogStar.magnitude is supposed to correspond to what you'd find in any astronomical database, while Star.magnitude is just supposed to be a relative measure of magnitude, since any estimate of magnitude from a centroiding algorithm is going to be pretty rough.
| catalog.erase(catalog.begin() + i - 1); | ||
| i--; |
There was a problem hiding this comment.
This i-- isn't necessary, and in fact if there were three identical stars in a row, the i-- would cause one of them to be missed.
| @@ -32,7 +32,7 @@ StarIdentifiers GeometricVotingStarIdAlgorithm::Go( | |||
| StarIdentifiers identified; | |||
| MultiDatabase multiDatabase(database); | |||
| const unsigned char *databaseBuffer = multiDatabase.SubDatabasePointer(PairDistanceKVectorDatabase::kMagicValue); | |||
| if (databaseBuffer == NULL) { | |||
| if (databaseBuffer == nullptr) { | |||
There was a problem hiding this comment.
like with typedef, there are dozens of uses of NULL throughout the codebase, if you're going to change one, change them all.
|
I agree with changes to the docs. See my comments about the rest. might be worth adding a unit test for the flitering stuff given the bugs |
|
Interesting -- on my M2 mac (ARM), the off-by-one error actually IS caught by the address sanitizer -- but not on my x86-64 laptop. |
Little things I picked up while doing a refresher over the codebase
io.cpp'sCatalogRead? Starting indexiatcatalog.size()would go out-of-bounds + iterator erase error. Also, if 2 stars are at the same position, we want to keep the one with greater magnitude (note: none of my changes changes the final catalog size (-70 stars))pipeline.man, I personally think it makes more sense to describe thedatabaseoption under star ID and optional centroiding arguments under Centroiding, instead of having the Misc section