Skip to content

Commit

Permalink
Fixed bug where focal plane measured x,y coordinates were not set if …
Browse files Browse the repository at this point in the history 
…the cam->SetImage call failed, References DOI-USGS#2591). (DOI-USGS#3193)

* updates to ControlPoint::ComputeApriori

* Fixed bug where focal plane measured x,y coordinates were not set if the cam->SetImage call failed (References DOI-USGS#2591). Added check to IsConstrained() method to see if point type is Free, in which case we ignore stored a priori sigmas on the coordinates.
  • Loading branch information
@kledmundson @scsides
kledmundson authored and scsides committed Mar 29, 2019
1 parent 4571f4f commit c3c6ac4
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 93 deletions.
221 changes: 129 additions & 92 deletions isis/src/control/objs/ControlPoint/ControlPoint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -839,11 +839,18 @@ namespace Isis {
}


/**
* This method computes the apriori lat/lon for a point. It computes this
* by determining the average lat/lon of all the measures. Note that this
* does not change ignored, or fixed points. Also, it does not
* use unmeasured or ignored measures when computing the lat/lon.
/**
* Computes a priori lat/lon/radius point coordinates by determining the average lat/lon/radius of
* all measures. Note that this does not change ignored, fixed or constrained points.
*
* Also, it does not use unmeasured or ignored measures when computing lat/lon/radius.
*
* (KLE) Note this not a rigorous triangulation considering outliers. A better way would be to...
* a) use e.g. a closest approach algorithm to find intersection of all rays, regardless
* of whether the intersection lies on the surface in question, then;
* b) perform a rigorous triangulation with some sort outlier detection approach, a robust
* estimation technique (perhaps RANSAC)
*
* @internal
* @history 2008-06-18 Tracie Sucharski/Jeannie Walldren,
* Changed error messages for
Expand Down Expand Up @@ -872,114 +879,88 @@ namespace Isis {
* @history 2017-04-25 Debbie A. Cook - change constraint status calls
* to use generic coordinate names (Coord1, Coord2,
* and Coord3).
* @history 2019-03-10 Ken Edmundson - Fixed bug where focal plane measured x,y coordinates were
* not set if the cam->SetImage call failed. Setting the measured
* focal plane coordinates should not depend upon the success of the
* SetImage call (References #2591). Improved error messages.
* Cleaned up code. Added comments above to suggest a more rigorous
* approach to computing a priori point coordinates.
*
* @return Status Success or PointLocked
*/
ControlPoint::Status ControlPoint::ComputeApriori() {
// TODO (KLE): where should call this go? Also, what's the point? The method has no description.
PointModified();

// Don't goof with fixed points. The lat/lon is what it is ... if
// it exists!
// 2013-11-12 KLE I think this check should include points with any
// number of constrained coordinates??? I agree DAC. *** TODO ***
if (GetType() == Fixed) {
if (!aprioriSurfacePoint.Valid()) {
QString msg = "ControlPoint [" + GetId() + "] is a fixed point ";
msg += "and requires an apriori x/y/z";
throw IException(IException::User, msg, _FILEINFO_);
}
// Don't return until after the FocalPlaneMeasures have been set
// return;
// if point is fixed or constrained, ensure valid a priori point coordinates exist
if ( (IsFixed() || IsConstrained()) && !aprioriSurfacePoint.Valid() ) {
QString msg = "In method ControlPoint::ComputeApriori(). ControlPoint [" + GetId() + "] is ";
msg += "fixed or constrained and requires a priori coordinates";
throw IException(IException::User, msg, _FILEINFO_);
}

double xB = 0.0;
double yB = 0.0;
double zB = 0.0;
double r2B = 0.0;
double xB = 0.0; // body-fixed x
double yB = 0.0; // body-fixed y
double zB = 0.0; // body-fixed z
double r2B = 0.0; // radius squared in body-fixed
int goodMeasures = 0;
double pB[3];

// Loop for each measure and compute the sum of the lat/lon/radii
for (int j = 0; j < cubeSerials->size(); j++) {
ControlMeasure *m = GetMeasure(j);

// The comment code was really checking for candidate measures
// Commented out 2011-03-24 by DAC
// if (!m->IsMeasured()) {
// // TODO: How do we deal with unmeasured measures
// }
// else if (m->IsIgnored()) {
// loop over measures to ...
// 1) set focal plane x,y coordinates for all unignored measures;
// 2) sum latitude, longitude, and radius coordinates in preparation for computing a priori
// coordinates by averaging.
for (int i = 0; i < cubeSerials->size(); i++) {
ControlMeasure *m = GetMeasure(i);
if (m->IsIgnored()) {
// TODO: How do we deal with ignored measures
continue;
}
else {
Camera *cam = m->Camera();
if (cam == NULL) {
QString msg = "The Camera must be set prior to calculating apriori";
throw IException(IException::Programmer, msg, _FILEINFO_);
}
if (cam->SetImage(m->GetSample(), m->GetLine())) {
goodMeasures++;
double pB[3];
cam->Coordinate(pB);
xB += pB[0];
yB += pB[1];
zB += pB[2];
r2B += pB[0]*pB[0] + pB[1]*pB[1] + pB[2]*pB[2];

double x = cam->DistortionMap()->UndistortedFocalPlaneX();
double y = cam->DistortionMap()->UndistortedFocalPlaneY();
m->SetFocalPlaneMeasured(x, y);
}
else {
// JAA: Don't stop if we know the lat/lon. The SetImage may fail
// but the FocalPlane measures have been set
if (GetType() == Fixed) {
continue;
}

// TODO: What do we do
// QString msg = "Cannot compute lat/lon/radius (x/y/z) for "
// "ControlPoint [" + GetId() + "], measure [" +
// m->GetCubeSerialNumber() + "]";
// throw IException(IException::User, msg, _FILEINFO_);

// m->SetFocalPlaneMeasured(?,?);
}
Camera *cam = m->Camera();
if (cam == NULL) {
QString cubeSN = m->GetCubeSerialNumber();
QString msg = "in method ControlPoint::ComputeApriori(). Camera has not been set in ";
msg += "measure for cube serial number [" + cubeSN + "], Control Point id ";
msg += "[" + GetId() + "]. Camera must be set prior to calculating a priori coordinates";
throw IException(IException::Programmer, msg, _FILEINFO_);
}
}

// Don't update the apriori x/y/z for fixed points TODO This needs a closer look
if (GetType() == Free ) {
// point can be tagged as "Free" but still have constrained coordinates
// if tagged "Free" we want to compute approximate a priori coordinates
bool setImageSuccess = cam->SetImage(m->GetSample(), m->GetLine());
m->SetFocalPlaneMeasured(cam->DistortionMap()->UndistortedFocalPlaneX(),
cam->DistortionMap()->UndistortedFocalPlaneY());

// TODO: Seems like we should be able to skip this computation if point is fixed or
// constrained in any coordinate. Currently we are always summing coordinates here. We could
// save time by not doing this for fixed or constrained points.
if (setImageSuccess) {
goodMeasures++;
cam->Coordinate(pB);
xB += pB[0];
yB += pB[1];
zB += pB[2];
r2B += pB[0]*pB[0] + pB[1]*pB[1] + pB[2]*pB[2];
}
}
// if point is "Fixed" or otherwise constrained in one, two, or all three
// coordinates, then we use the a priori surface point coordinates that
// have been given via other means (e.g. through qnet or cneteditor)
// 2013-11-12 KLE Is the next check better as "if Fixed or if # of
// constrained coordinates > 1" ???
else if (GetType() == Fixed
|| NumberOfConstrainedCoordinates() == 3
|| IsCoord1Constrained()
|| IsCoord2Constrained()
|| IsCoord3Constrained()) {

// Initialize the adjusted x/y/z to the a priori coordinates
// if point is Fixed or Constrained in any number of coordinates, initialize adjusted surface
// point to a priori coordinates (set in e.g. qnet or cneteditor) and exit
if( IsFixed() || IsConstrained()) {
adjustedSurfacePoint = aprioriSurfacePoint;

return Success;
}

// Beyond this point, we assume the point is free ***TODO*** confirm this
// Did we have any measures?
// if point is Free, we continue to compute a priori coordinates

// if no good measures, we're done
// TODO: is the message true/meaningful?
if (goodMeasures == 0) {
QString msg = "ControlPoint [" + GetId() + "] has no measures which "
"project to lat/lon/radius (x/y/z)";
QString msg = "in method ControlPoint::ComputeApriori(). ControlPoint [" + GetId() + "] has ";
msg += "no measures which project to the body";
throw IException(IException::User, msg, _FILEINFO_);
}

// Compute the averages if all coordinates are free
//if (NumberOfConstrainedCoordinates() == 0) {
// TODO: confirm if this "if" statement is necessary
if (GetType() == Free || NumberOfConstrainedCoordinates() == 0) {
double avgX = xB / goodMeasures;
double avgY = yB / goodMeasures;
Expand Down Expand Up @@ -1553,11 +1534,6 @@ namespace Isis {
}


bool ControlPoint::IsFixed() const {
return (type == Fixed);
}


SurfacePoint ControlPoint::GetAprioriSurfacePoint() const {
return aprioriSurfacePoint;
}
Expand All @@ -1581,22 +1557,78 @@ namespace Isis {
}


/**
* Return bool indicating if point is Free or not.
*
* @return bool indicating if point is Free or not.
*/
bool ControlPoint::IsFree() const {
return (type != Fixed && type != Constrained);
}


/**
* Return bool indicating if point is Fixed or not.
*
* @return bool indicating if point is Fixed or not.
*/
bool ControlPoint::IsFixed() const {
return (type == Fixed);
}


/**
* Return bool indicating if point is Constrained or not.
*
* @return bool indicating if point is Constrained or not.
*/
bool ControlPoint::IsConstrained() {
// if point type is Free, we ignore any a priori sigmas on the coordinates
if (type == Free) {
return false;
}

return constraintStatus.any();
}


/**
* Return bool indicating if 1st coordinate is Constrained or not. This corresponds to Latitude
* for a Latitudinal solution or X for a Rectangular solution.
*
* @return bool indicating if 1st coordinate is Constrained or not.
*/
bool ControlPoint::IsCoord1Constrained() {
return constraintStatus[Coord1Constrained];
}


/**
* Return bool indicating if 2nd coordinate is Constrained or not. This corresponds to Longitude
* for a Latitudinal solution or Y for a Rectangular solution.
*
* @return bool indicating if 2nd coordinate is Constrained or not.
*/
bool ControlPoint::IsCoord2Constrained() {
return constraintStatus[Coord2Constrained];
}

/**
* Return bool indicating if 3rd coordinate is Constrained or not. This corresponds to Radius
* for a Latitudinal solution or Z for a Rectangular solution.
*
* @return bool indicating if 3rd coordinate is Constrained or not.
*/
bool ControlPoint::IsCoord3Constrained() {
return constraintStatus[Coord3Constrained];
}


/**
* Return bool indicating if point is Constrained or not.
*
* @return bool indicating if point is Constrained or not.
*/
int ControlPoint::NumberOfConstrainedCoordinates() {
return constraintStatus.count();
}
Expand All @@ -1616,6 +1648,7 @@ namespace Isis {
return aprioriRadiusSourceFile;
}


ControlPoint::SurfacePointSource::Source
ControlPoint::GetAprioriSurfacePointSource() const {
return aprioriSurfacePointSource;
Expand Down Expand Up @@ -1990,6 +2023,10 @@ namespace Isis {
}


/**
* What the heck is the point of this?
*
*/
void ControlPoint::PointModified() {
dateTime = "";
}
Expand Down
7 changes: 6 additions & 1 deletion isis/src/control/objs/ControlPoint/ControlPoint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,10 @@ namespace Isis {
* Fixes #5435.
* @history 2018-06-30 Debbie A. Cook Removed all calls to obsolete method
* SurfacePoint::SetRadii. References #5457.
* @history 2019-03-10 Ken Edmundson - See history entry for ComputeApriori method (References
* #2591). Added check to IsConstrained() method to see if point type is
* Free, in which case we ignore stored a priori sigmas on the
* coordinates.
*/
class ControlPoint : public QObject {

Expand Down Expand Up @@ -529,6 +533,7 @@ namespace Isis {
bool IsValid() const;
// Can we get rid of this? It doesn't appear to be used anywhere. *** ToDo ***
bool IsInvalid() const;
bool IsFree() const;
bool IsFixed() const;
bool HasAprioriCoordinates();

Expand Down Expand Up @@ -668,7 +673,7 @@ namespace Isis {
* This stores the constraint status of the a priori SurfacePoint
* @todo Eventually add x, y, and z. Instead we made generic coordinates
*/
std::bitset<6> constraintStatus;
std::bitset<3> constraintStatus;

/**
* This indicates if a program has explicitely set the reference in this
Expand Down

0 comments on commit c3c6ac4

Please sign in to comment.