-
Notifications
You must be signed in to change notification settings - Fork 169
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
Marcical update to be fully congruent with calibration method #5004
Conversation
The tests that are failing are unrelated to the changes created in this PR. I am not sure if these test failures are expected however. |
filterNameToFilterIndex.insert(pair<QString, int>("NIR", 5)); | ||
filterNameToFilterIndex.insert(pair<QString, int>("SHORT_UV", 6)); | ||
filterNameToFilterIndex.insert(pair<QString, int>("LONG_UV", 7)); | ||
map<QString, int> filterNameToFilterNumber; |
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.
The filter index used in this code is zero based, so renamed for accuracy.
@@ -347,21 +337,14 @@ namespace Isis { | |||
PvlKeyword &colOff = icube.label()->findGroup("Instrument", Pvl::Traverse)["ColorOffset"]; | |||
int colorOffset = toInt(colOff[0]); | |||
|
|||
for (int filter = 0; filter < numFilters; filter++) { | |||
for (int i = 0; i < numFilters; i++) { |
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.
filter variable is set previously, it did not seem to be resetting the larger variable, but changed for readability
if ((*fcubeMgrs[fcubeIndex])[i] < 0.25) { | ||
ocubeMgr[i] = 0; | ||
} | ||
else { | ||
ocubeMgr[i] = stretch.Map(icubeMgr[i]) / (*fcubeMgrs[fcubeIndex])[i]; | ||
} |
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 is the portion of the calibration logic missing from our implementation
See step 3 of https://pds-imaging.jpl.nasa.gov/data/mro/mars_reconnaissance_orbiter/marci/mrom_1343/calib/marcical.txt
@@ -524,6 +512,11 @@ namespace Isis { | |||
icubeMgr++; | |||
ocubeMgr++; | |||
|
|||
// checks if we have reached the end before setting higher band | |||
if (ocubeMgr.end()) { |
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.
Prevents non-existent band from being set in the last chunk of code in the loop.
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.
Make sure you add an entry to the changelog and to the marcical.xml file.
c2db8c6
to
73b00cb
Compare
CHANGELOG.md
Outdated
@@ -34,6 +34,7 @@ release. | |||
--> | |||
|
|||
## [Unreleased] | |||
- Update marcical to include step 3 of the mission team's MARCI calibration process described [here](https://pds-imaging.jpl.nasa.gov/data/mro/mars_reconnaissance_orbiter/marci/mrom_1343/calib/marcical.txt). [#5004](https://github.com/USGS-Astrogeology/ISIS3/pull/5004) |
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.
Put this under an appropriate header
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 do! I will move the other entries while I am at it!
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.
Looks good to go in
@ladoramkershner Go ahead and merge then when you're ready to add the new data |
1 similar comment
@ladoramkershner Go ahead and merge then when you're ready to add the new data |
One small functional update to make sure we are fully implimenting the MARCI calibration method outlined here. Specifically step 3 is being implemented in this PR.
Description
In the MARCI calibration documentation output by the MRO mission team, after the flat file is taken through normalization and flattening there should be a check applied. If the flat file value is below 0.25 the equation in the follow step (step 4) should be set to zero. This was not reflected in the code.
All other changes are syntax improvements.
Related Issue
Related to issue #4619
Motivation and Context
Our MARCI calibration was not fully congruent to what the mission teams outlined for calibration of MARCI images.
How Has This Been Tested?
Output from running tests locally
Screenshots (if appropriate):
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: