Jigsaw fixes #5833
Jigsaw fixes #5833
Conversation
|
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5833". Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch. |
|
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5833". Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch. |
acpaquette
left a comment
acpaquette
left a comment
There was a problem hiding this comment.
Overall looks good! A couple of user facing questions that could use some discussion
| "failure may result."; | ||
| PvlGroup radiusSolveWarning("RadiusSolveWarning"); | ||
| radiusSolveWarning.addKeyword(PvlKeyword("Warning", msg.c_str())); | ||
| Application::Log(radiusSolveWarning); |
There was a problem hiding this comment.
This should use Application::AppendAndLog for CLI and application compatibility
| QString msg = "In method ControlPoint::ComputeApriori(): ControlPoint [" + GetId() + "] has "; | ||
| msg += "no measures which project to the body. Will ignore it."; | ||
| SetIgnored(true); | ||
| std::cerr << msg << std::endl; |
There was a problem hiding this comment.
I like the warning but for most users this will still dump to the terminal, or most won't pipe the error message to another output. We need a way to silence the warnings in the preference file or some other setting. @Kelvinrr Thoughts on this? I know we had issues when we added a deprecation warning for users, not sure what discussions came out of that
There was a problem hiding this comment.
Sometimes many control points will fail to work. I am not sure what you have in mind, but we probably would not want a popup for each of them. This could even be silent, with failed points simply glossed over. Fine with me whichever way this would be dealt with.
There was a problem hiding this comment.
yeah we almost universally get complaints about warnings, it can mess up people who parse app outputs which is very common since ISIS often returns PVL. It might be possible to use the log for this since that can be suppressed?
There was a problem hiding this comment.
Can you suggest what this line should be changed to? There appear to be various choices in several places.
There was a problem hiding this comment.
Hi All - could someone please be more explicit about this what is being discussed here as this is a program my job depends on and changes have impact. Is this a situation where measures for a point fail to intersect the body? If so, these should not be silenced and the program either needs to fail outright (as it does now) and/or that information should go to a log file. I want to know about these situations as they indicate some sort of a problem and should be removed and/or my network building needs additional checks. More importantly I need to understand why it is happening and hiding the error does not give me that opportunity.
It is recommended that users run cnetcheck on their network prior to passing it to jigsaw which will identify measures that do not intersect the target via the nolatlon output file.
There was a problem hiding this comment.
Will cnetcheck go through all the same motions as jigsaw? Can then the failure message refer to these two?
There was a problem hiding this comment.
cnetcheck will report if the measure does not intersect body when nolatlon=True (default) and send the information to an output file (which is not the most user friendly format so you might need a script to parse the output to send to cnetedit to remove the offending measures). It doesn't iterate like jigsaw, it will only check the state of the network based on spice and the line/sample information of the measures (probably similar to what jigsaw does before it starts to iterate). But it will report on off target measures along with free points having single measures, points having nocontrol (because all measures are invalid (ignored), etc.
cnetcheck is more robust that cnetstats.
It might be useful if jigsaw referred the user to cnetcheck in the case where a measure does not intersect the target body. But I'm not sure the particular bundle error I'm referring to for off-body targets is the same as what you are experiencing with your networks.
I won't go into all of the detail, but this is the jigsaw failure output for voyager data where measures essentially had no spice because they were off the target:
Group = Error
Program = jigsaw
Class = "USER ERROR"
Code = 2
Message = "Unable to bundle adjust network [tb_vgr_voygalnet_Epos.net]"
File = main.cpp
Line = 157
End_Group
Group = Error
Program = jigsaw
Class = "USER ERROR"
Code = 2
Message = "Could not solve bundle adjust"
File = BundleAdjust.cpp
Line = 982
End_Group
Group = Error
Program = jigsaw
Class = "USER ERROR"
Code = 2
Message = "Unable to map apriori surface point for measure
Voyager2/NAC/2064922 on point 133 into focal plane"
File = BundleAdjust.cpp
Line = 1887
End_Group
I can't tell if this is the same sort of error you were encountering with your Juno data though.
I will add that for my Voyager and some Galileo disk data of Europa we attempted to avoid this sort of problem by manually moving the spice onto the target for images having issues. Very user intensive, but the process works (I can share those details in an email if you are interested and if applicable to your particular situation).
I have also encountered problems with Galileo SSI Europa, Cassini ISS Enceladus, Titan and MEX SRC and HRSC Phobos data where points end up right on the limb for images (having spice mostly on the target) because a measure is a fraction away of having spice or not. Cnetcheck has found those instances (reported in nolatlon output) and they are removed from the network prior to running the network through the bundle adjustment. If this is more in line with what you are experiencing with you JunoCam data you might see if cnetcheck spots your known problems.
And so I understand better, in the suggested warning message
QString msg = "In method ControlPoint::ComputeApriori(): ControlPoint [" + GetId() + "] has ";
msg += "no measures which project to the body. Will ignore it.";
SetIgnored(true);
std::cerr << msg << std::endl;
Does "ignore" mean literally setting a measure in the network to ignore or does it simply mean not using it in calculations during any or all iterations?
If it is that latter (either really, the user needs to know if jigsaw is modifying the state of measures), maybe it could be flagged as "rejected" which would show up in the residuals.csv file in a similar way as measures are via the use of OUTLIER_REJECTION. But the user would need to know to look in the residuals file for rejected measures and be able to discern rejection due to outlier_rejection use versus rejection due to failing to intersect the target. That seems messy, so maybe ignore that idea. Which is why I think depending on jigsaw to validate a network should be discouraged and cnetcheck should be run to catch some of the most egregious issues before hand.
There was a problem hiding this comment.
Does "ignore" mean literally setting a measure in the network to ignore or does it simply mean not using it in calculations during any or all iterations?
The control point will be flagged as ignored in the control network, and will be skipped when forming the optimization problem.
I will add that for my Voyager and some Galileo disk data of Europa we attempted to avoid this sort of problem by manually moving the spice onto the target for images having issues. Very user intensive, but the process works
This is likely not a great solution. Jigsaw should have the ability to filter out various problematic points without failing. It should print a report of the fraction of points that cannot be processed.
I rarely use this, so will defer to actual users and maintainers, but I think this is a case of a tool that should be more robust and give the user more choices of how to handle borderline data.
There was a problem hiding this comment.
This is likely not a great solution. Jigsaw should have the ability to filter out various problematic points without failing. It should print a report of the fraction of points that cannot be processed.
No, not fun, but in the case of Voyager data of Europa there was no choice - the spice was completely off the target and had to be moved. Extreme case.
I don't completely disagree that jigsaw should be capable of ignoring problem measures, reporting on it and continuing to iterate if possible. However it may be tough parsing the various possible errors (with some level of specificity) into a single new output report.
And I apologize for commenting on these possible changes so late in the process. I missed the original issue post and only saw the PR today.
There was a problem hiding this comment.
However it may be tough parsing the various possible errors (with some level of specificity) into a single new output report.
One has to start somewhere. Ignoring failed control points and adding a report file is a good place. But since regular users seem not to see it as a problem, I guess maybe it is fine, most of the time.
BTW, for JunoCam I ran into systematic problems in produced terrain models that were due to intrinsics. ASP can tune those, and we have a doc.
|
Quick comment regarding the underconstrained warning. We certainly need this in some form, but I suspect that this check is insufficient. I'm not sure that it takes into consideration constraints on individual control point coordinates that may satisfy (or more than satisfy) the datum defect of 7 (set for example in cneteditor or qnet). Or constraints that have been placed on spk position and/or pointing. |
|
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5833". Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch. |
|
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5833". Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch. |
|
@kledmundson I am not familiar enough with the tool to say for sure. I guess user expertise is needed in either case, and if the user knows some constraints were already set, they may choose to set this one to a relaxed value or simply ignore the warning. Any sensible use of this tool likely needs some ground constraint in either case (and likely constrain at least 3 points), as for very high focal length any constraint on camera position and orientation alone may still result in too much of displacement on the ground. |
|
Thank you for your contribution! Unfortunately, this pull request hasn't received much attention lately, so it is labeled as 'stale.' If no additional action is taken, this pull request will be automatically closed in 180 days. If you want to participate in our support prioritization meetings or be notified when support sprints are happening, you can sign up the support sprint notification emails here. Read more about our support processs here |
github-actions
bot
added
the
inactive
5 participants
Description
These are proposed fixes for jigsaw issues #5821 and #5818.
How Has This Been Validated?
I tested with a control network I had.
Types of changes
Checklist:
Licensing
This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words: