Skip to content
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.

Sign up for GitHub

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

Code sweep #122

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Code sweep #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f536a2a
Potential (minor) bug fix, refactor
zeddie888 Oct 22, 2023
db22107
smol refactor
zeddie888 Oct 22, 2023
9ddbed2
Edit pipeline man page, move commands out of Misc
zeddie888 Oct 22, 2023
bebab20
fix iterator erase-read conflict
zeddie888 Oct 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions documentation/pipeline.man
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.TH PIPELINE 3 "06 November 2021"
.TH PIPELINE 3 "06 November 2021"

.SH NAME

Expand Down Expand Up @@ -88,21 +88,6 @@ The field of view of the camera that took the picture (in degrees). Defaults to

.SH MISC PIPELINE OPTIONS

.TP
\fB--centroid-mag-filter\fP \fImin-mag\fP
Will not consider centroids with magnitude below \fImin-mag\fP.

.TP
\fB--centroid-filter-brightest\fP \fInum-stars\fP
Remove all but the brightest \fInum-stars\fP many stars from the list of centroids before sending to
star-id. Often a better choice than \fB--centroid-mag-filter\fP, because you can ensure that you
keep enough stars to do star-id. If both this option and \fB--centroid-mag-filter\fP are provided,
then all stars satisfying both criteria are kept (intersection).

.TP
\fB--database\fP \fIfilename\fP
Chooses \fIfilename\fP as the database to use during star identification.

.TP
\fB--help\fI
Prints the contents of the manual entry for the command to the terminal.
Expand All @@ -117,12 +102,27 @@ Runs the \fIalgo\fP centroiding algorithm. Recognized options are: dummy (random
\fB--centroid-dummy-stars\fP \fInum-stars\fP
Runs the dummy centroiding algorithm (random centroid algorithm) with \fInum-stars\fP stars centroided. Defaults to 5 if option is not selected.

.TP
\fB--centroid-mag-filter\fP \fImin-mag\fP
Argument is optional. Will not consider centroids with magnitude below \fImin-mag\fP.

.TP
\fB--centroid-filter-brightest\fP \fInum-stars\fP
Argument is optional. Remove all but the brightest \fInum-stars\fP many stars from the list of centroids before sending to
star-id. Often a better choice than \fB--centroid-mag-filter\fP, because you can ensure that you
keep enough stars to do star-id. If both this option and \fB--centroid-mag-filter\fP are provided,
then all stars satisfying both criteria are kept (intersection).

.SH STAR IDENTIFICATION OPTIONS

.TP
\fB--star-id-algo\fP \fIalgo\fP
Runs the \fIalgo\fP star identification algorithm. Current options are "dummy", "gv", and "pyramid". Defaults to "dummy" if option is not selected.

.TP
\fB--database\fP \fIfilename\fP
Chooses \fIfilename\fP as the database to use during star identification.

.TP
\fB--angular-tolerance\fP [\fItolerance\fP] Sets the estimated angular centroiding error tolerance,
used in some star id algorithms, to \fItolerance\fP degrees. Defaults to 0.04 degrees.
Expand Down
4 changes: 2 additions & 2 deletions src/databases.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const int32_t kCatalogMagicValue = 0xF9A283BC;

/**
* A data structure enabling constant-time range queries into fixed numerical data.
*
*
* @note Not an instantiable database on its own -- used in other databases
*/
// TODO: QueryConservative, QueryExact, QueryTrapezoidal?
Expand Down Expand Up @@ -114,7 +114,7 @@ class MultiDatabaseEntry {
std::vector<unsigned char> bytes;
};

typedef std::vector<MultiDatabaseEntry> MultiDatabaseDescriptor;
using MultiDatabaseDescriptor = std::vector<MultiDatabaseEntry>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you choose to change this one typedef among all the typedefs. There's probably about a dozen other typedefs


void SerializeMultiDatabase(SerializeContext *, const MultiDatabaseDescriptor &dbs);

Expand Down
22 changes: 15 additions & 7 deletions src/io.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,24 @@ const Catalog &CatalogRead() {
char *tsvPath = getenv("LOST_BSC_PATH");
catalog = BscParse(tsvPath ? tsvPath : DEFAULT_BSC_PATH);
// perform essential narrowing
// remove all stars with exactly the same position as another, keeping the one with brighter magnitude
// remove all stars with exactly the same position as another, keeping the one with brighter
// magnitude
std::sort(catalog.begin(), catalog.end(), [](const CatalogStar &a, const CatalogStar &b) {
return a.spatial.x < b.spatial.x;
if (a.spatial.x != b.spatial.x) {
return a.spatial.x < b.spatial.x;
} else if (a.spatial.y != b.spatial.y) {
return a.spatial.y < b.spatial.y;
}
return a.spatial.z < b.spatial.z;
});
for (int i = catalog.size(); i > 0; i--) {
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) {
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.
Copy link
Member

@markasoftware markasoftware Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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--;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
catalog.erase(catalog.begin() + i);
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions src/star-id.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like with typedef, there are dozens of uses of NULL throughout the codebase, if you're going to change one, change them all.

return identified;
}
DeserializeContext des(databaseBuffer);
Expand Down Expand Up @@ -173,7 +173,7 @@ class PairDistanceInvolvingIterator {
* If another PairDistanceInvolvingIterator is equal to this, then it is done iterating.
*/
PairDistanceInvolvingIterator()
: pairs(NULL), end(NULL) { };
: pairs(nullptr), end(nullptr) { };

/**
* The main constructor.
Expand Down Expand Up @@ -252,7 +252,7 @@ std::vector<int16_t> ConsumeInvolvingIterator(PairDistanceInvolvingIterator it)
/**
* Given the result of a pair-distance kvector query, build a hashmultimap of stars to other stars
* that appeared with it in the query.
*
*
* The resulting map is "symmetrical" in the sense that if a star B is in the map for star A, then
* star A is also in the map for star B.
*/
Expand Down Expand Up @@ -447,7 +447,7 @@ IRUnidentifiedCentroid *SelectNextUnidentifiedCentroid(std::vector<IRUnidentifie
return result;
}

return NULL;
return nullptr;
}

const float kAngleFrom90SoftThreshold = M_PI_4; // TODO: tune this
Expand Down Expand Up @@ -510,7 +510,7 @@ int IdentifyRemainingStarsPairDistance(StarIdentifiers *identifiers,
while (!belowThresholdUnidentifiedCentroids.empty() || !aboveThresholdUnidentifiedCentroids.empty()) {
IRUnidentifiedCentroid *nextUnidentifiedCentroid
= SelectNextUnidentifiedCentroid(&aboveThresholdUnidentifiedCentroids, &belowThresholdUnidentifiedCentroids);
if (nextUnidentifiedCentroid == NULL) {
if (nextUnidentifiedCentroid == nullptr) {
break;
}

Expand Down Expand Up @@ -574,7 +574,7 @@ StarIdentifiers PyramidStarIdAlgorithm::Go(
StarIdentifiers identified;
MultiDatabase multiDatabase(database);
const unsigned char *databaseBuffer = multiDatabase.SubDatabasePointer(PairDistanceKVectorDatabase::kMagicValue);
if (databaseBuffer == NULL || stars.size() < 4) {
if (databaseBuffer == nullptr || stars.size() < 4) {
std::cerr << "Not enough stars, or database missing." << std::endl;
return identified;
}
Expand Down Expand Up @@ -643,7 +643,7 @@ StarIdentifiers PyramidStarIdAlgorithm::Go(
/ std::max(std::max(iSinInner, jSinInner), kSinInner);

if (expectedMismatches > maxMismatchProbability) {
std::cout << "skip: mismatch prob." << std::endl;
std::cerr << "skip: mismatch prob." << std::endl;
continue;
}

Expand Down
Loading