-
Notifications
You must be signed in to change notification settings - Fork 171
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
Save and apply jigsaw adjustments #5419
Save and apply jigsaw adjustments #5419
Conversation
* @param tableStr The table string | ||
* @param fieldDelimiter The delimiter to separate fields with | ||
*/ | ||
Table::Table(const QString &tableName, const std::string &tableString, const char &fieldDelimiter) { |
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 think if we are reading in Jigsaw results we should be able to use the ISIS CSVReader class instead of implementing the reader in Table. I could be missing something about our CSV Reader though.
Something to try when we get back to this
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.
CSVReader was kind of a mess. I dont mind re-visiting trying it, originally, we wanted to just write out the blobs using with something like Table -> toBlob -> write(stringstream) -> write to HDF5. But that also was a mess. If we revisit I think trying the blob route would be best. She was able to get it work with a quick ghetto method than I was trying to de-tangle Blob writing with streams. Only spent a day on it, but I was getting a bunch of segfaults and stream errors for whatever reason.
Unable to run tests until merge conflicts have been handled. |
Running into dependency errors:
|
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5419". 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_5419". 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_5419". 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. |
isis/tests/FunctionalTestsJigsaw.cpp
Outdated
@@ -486,7 +491,8 @@ TEST_F(ObservationPair, FunctionalTestJigsawCamSolveAll) { | |||
QString outCnetFileName = prefix.path() + "/outTemp.net"; | |||
QVector<QString> args = {"fromlist="+cubeListFile, "cnet="+cnetPath, "onet="+outCnetFileName, | |||
"observations=yes", "update=yes", "Cksolvedegree=3", | |||
"Camsolve=all", "twist=no", "Spsolve=none", "Radius=no", "imagescsv=on", "file_prefix="+prefix.path()+"/"}; | |||
"Camsolve=all", "twist=no", "Spsolve=none", "Radius=no", "imagescsv=on", "file_prefix="+prefix.path()+"/", | |||
"adjustment_output=adj_temp.h5"}; |
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.
Were these tests failing because it was looking for adjustment_output? I would assume that this parameter should be optional. Otherwise, this is a breaking change.
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 can be changed to optional.
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5419". 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. |
I have done a little bit of testing on this. Invocation
DocumentationWhen applying the adjustment, what parameters are needed? I assume only: cnet, onet, fromlist, adjustment_input, file_prefix? What happens if I specify other parameters? Functionality
Future WorkI think there is an opportunity in the future for improved interoperability with ASP. In ASP, one can run pc_align and then a single iteration adjustment to update the state of the input sensor. The input to that is the adjustment transformation needed to shift the ephemerides. I assume that is exactly what is being stored in the H5 file. Somehow coordinating and matching formats of the stored adjustments would mean that one could bundle in ISIS and then update CSM state files in ASP. |
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5419". 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_5419". 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_5419". 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_5419". 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. |
Looks like some jigsaw tests are failing |
@chkim-usgs Looks like the apollo jigsaw failures are related to HDF5 file writing and the CSM jigsaw test is failing as it's trying to access elements not available on the CSM init'd cube. |
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5419". 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. |
<parameter name="ADJUSTMENT_INPUT"> | ||
<type>filename</type> | ||
<internalDefault>none</internalDefault> | ||
<fileMode>input</fileMode> | ||
<brief> | ||
Reads in and applies adjustment values | ||
</brief> | ||
<description> | ||
Reads in and applies bundle adjustment values from an HDF5 file, | ||
usually the outputted adjustment_out.h5 file. Along with the three | ||
required parameters, UPDATE must be set to YES for the input values | ||
to be read and applied. | ||
</description> | ||
<filter> | ||
*.h5 | ||
</filter> | ||
</parameter> |
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 feel we should exclude other files if this is passed in? Things like the adjustment parameters and ADJUSTMENTOUT_H5
. See line 1561 on this XML for an example.
Slightly related, @amystamile-usgs has the app XML docs been transferred? I couldn't find it in the docs. If not it's fine it seems like it needs some work.
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.
Can you help specify which params need to be excluded? I was thinking most since we're just applying the adjustment input values and no other calculations should/could be done during this anyway?
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.
yeaaaaahhh, it'll pretty much be everything that isn't the input list I think.
@@ -1835,6 +1839,36 @@ | |||
<item>no</item> | |||
</default> | |||
</parameter> | |||
|
|||
<parameter name="ADJUSTMENTOUT_H5"> |
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.
Just to be more consistent with how ISIS names parameters, for better or worse, maybe name this OutAdjustmentH5
converges and the <b>UPDATE</b> parameter is selected. In addition to <b>UPDATE</b> being | ||
true and bundle converging, if the <b>ADJUSTMENT_INPUT</b> is set to an HDF5 file containing | ||
bundle adjustment values (usually the HDF5 output file from <b>ADJUSTMENTOUT_H5</b>), those | ||
values can be applied to the <def>cube</def> labels. |
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 would go more into detail about how this file is formatted in case someone wants to open it outside of the context of this app.
The build and test suite have started for your pull request. To view your build log, please reference the build with source version: "PR_5419". 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. |
Description
Added the boolean param
ADJUSTMENTOUT_H5
that is set totrue
as default and outputs the bundle adjustment values to an HDF5 file called adjustment_out.h5.Also created param
ADJUSTMENT_INPUT
that takes in an HDF5 file that holds the adjustment values to be applied to the cubes. For this functionality to occur,UPDATE
must be set totrue
.*Note that this new feature only currently support ISIS adjustments. The issue to add CSM support is here.
Related Issue
#4474, #5411
How Has This Been Validated?
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: