With new calibration knowledge, hical needs an update to (or alternative to) its dark current correction (ZeroDark) module.

[otherorb]

ISIS version(s) affected: All

Description
The ISIS3 hical program was written early in the MRO mission, before it became necessary to increase the operating temperature of the HiRISE instrument to improve image quality and reduce problematic hardware behaviors as the instrument has degraded over time. Dark current (thermal noise) in the HiRISE CCDs increases exponentially with temperature, and is not being properly corrected in HiRISE images taken at higher temperatures. Therefore, the dark current correction code in hical must be modified to improve HiRISE calibration accuracy. This modification is urgently needed, as poorly-calibrated data have been and continue to be released to the public and science community. A new dark current correction has been derived and is ready for implementation and testing within hical.

How to reproduce
This isn’t a software problem, so there’s no simple way to “reproduce” this. This is more of a correction to previously-acceptable behavior issue. Ultimately, if you run hical on any high-temperature HiRISE image after ingestion (hi2isis) the DN (or I/F) values that result from the calibration in the output image will look okay, but do not reflect the best possible calibration that could be achieved.

Possible Solution
Proposed patches are being worked at this time and will be uploaded after testing external to USGS ISIS3 development. A fork/branch where we are working on this issue can be found here: https://github.com/otherorb/ISIS3/tree/ZeroDarkRate and when it is more mature, we will submit a PR to the USGS repo. Contributions and participation (via PRs to the fork/branch) are welcome, but to keep discussions centralized, let's keep the discussion here in this issue, please.
We believe that, at a minimum, the following files may require some changes:
.../mro/objs/HiCal/ZeroDarkRate.h (new file, implements the new calibration algorithm)
.../mro/objs/HiCal/HiCalConf.h (patch)
.../mro/objs/HiCal/Module.h (patch)
.../mro/aps/hical/main.cpp (patch)
There will also need to be new files added to the DATA directory and a patch made and version increment to the hical.conf file.

[rbeyer]

In case there is any question, the work on the otherorb fork/branch mentioned above is being carried out under the auspices of the HiRISE Calibration Team, such that the to-be-PRed code represents not only the best practice (which honestly is the only thing that should matter here), but also happens to be the desire of the instrument team.

Sincerely,

• Ross Beyer (HiRISE Co-I)
• Ken Herkenhoff (HiRISE Co-I, HiRISE Calibration Lead)

[otherorb]

I realize the "How to reproduce" section in the original issue is somewhat lacking in useful information. I've attached a gif that might help better illustrate the issue. This is a comparison between: the raw image (no calibration---to illustrate one of the main calibration challenges); the existing calibration result (hical run to the point of removing the dark current); and the proposed update to the dark current correction.
On the right-hand side of the image as seen here, there's a brighter streak that runs down the length of the image. This is due to an amplifier, very near this section of the CCD, which heats up during imaging. This causes additional dark current to be generated in the CCD. The raw image shows the streak quite clearly. The current hical implementation corrects some of the excess dark current, but not all. The new dark current correction improves upon the current hical correction.

This is observation ESP_068088_0950, CCD6, Channel 1.

[jessemapel]

A new dark current correction has been derived and is ready for implementation and testing within hical.

Is this available anywhere? I'm not sure how to help out with any of this without the new correction and a description of how it's going to be implemented.

[rbeyer]

A new dark current correction has been derived and is ready for implementation and testing within hical.

Is this available anywhere? I'm not sure how to help out with any of this without the new correction and a description of how it's going to be implemented.

Thanks for being willing to help, and we may need it, but we're not asking for external help ... yet. @otherorb and I are starting to work the problem in the designated fork/branch, I think I properly assigned us to this Issue. Our intent is to work this from our side. If we encounter no difficulties, we'll just PR it (yay). However, if we run into issues, then we'll post them here and solicit help.

[otherorb]

Alright.
On my fork/branch (https://github.com/otherorb/ISIS3/tree/ZeroDarkRate), I've implemented the algorithm and addressed some of the bookkeeping and etc. There are a couple of places where I don't fully grok the ISIS3 API so will need some help:

1. I don't fully understand the LoadCSV module used in other places by the hical code. I've put a compile-stop in the place where I think that will need help.
2. I don't quite know where to put some logic to check if both ZeroDark and ZeroDarkRate are activated. The code should exit with a USER error if both modules are turned on.

Separately from the API and programming:
I have a bunch of CSV files that will need to be placed in the ISIS3/data/mro/calibration directory somewhere.

[jessemapel]

1. I don't fully understand the LoadCSV module used in other places by the hical code. I've put a compile-stop in the place where I think that will need help.

Can you provide a link to the place in code you are talking about? There's over a dozen files to look through in hical.

2. I don't quite know where to put some logic to check if both ZeroDark and ZeroDarkRate are activated. The code should exit with a USER error if both modules are turned on.

I would check at either config file read-in in HiCalConf or when you go to apply the ZeroDarkRate step in the main.

Separately from the API and programming:
I have a bunch of CSV files that will need to be placed in the ISIS3/data/mro/calibration directory somewhere.

If these files can be added without any conflicts, go ahead and send them to us or point us to the files that need to be added. We can get those added to the data area.

[otherorb]

Hi Jesse.
The module I'm referencing is here:
https://github.com/otherorb/ISIS3/blob/ZeroDarkRate/isis/src/mro/objs/HiCal/ZeroDarkRate.h

The specific part I'm struggling with starts on line 98. My hacky way to indicate the problem is to put a DEBUG line in the code so it won't compile and someone who knows what they're doing can help fix the next part.

If what I'm trying to accomplish here is unclear, please let me know.

	// Cause this file not to compile so that someone who knows what they're doing
	// can make this next part work.
	DEBUG
	// Load the coefficients 
	// I don't really know how loadCsv works. The CSV files for this module are named:
	// file: DarkRate_CCD_Ch_TDI${tdi}_BIN{$binning}_hical_????.csv
	// Example:
	// DarkRate_RED1_1_TDI64_BIN2_hical_0002.csv
	// The format of the file is as follows:
	// There are three header lines, with a '#' sign as a comment:
	// # Number of files used to generate these values = 40 
	// # exponential equation: DC_Rate = a * exp(b * FPA Temperature) + c 
	// # a, b, c 
	// Then the coefficients begin.
	// Three columns (a, b, c), and 1024/binning rows
	// 2.483618177203812394e+00,2.255885064806690821e-01,5.617339162650616345e+03
	// I don't know how loadCsv can be used to load this into memory.
	// Later in this file, I assume I have three coefficient variables:
	// _coeff_a, _coeff_b, and _coeff_c
	// Each of these variables is a vector with 1024/binning elements.
        _coeffs = loadCsv("DarkRate", conf, prof, samples);
	_coeff_a = _coeffs[0];
	_coeff_b = _coeffs[1];
	_coeff_c = _coeffs[2];

[otherorb]

I will send a tarball for upload to the USGS internal drives; none of those data files conflict with any existing data files.

[jessemapel]

Okay I took a look at LoadCSV and it should be able to ready your CSV into a matrix. The three lines at the start of the CSV are comments, not headers. So, if you set IgnoreComments in the config, LoadCSV should automatically skip them. From there, it will load all of the data into a HiMatrix which you can get from LoadCSV::getMatrix. This is just a TNT::Array2D so you can access values via matrix[row][column].

Hope that helps! Let me know if there's something I missed.

[jessemapel]

Here's a brief code snippet assuming that samples is the row of the CSV you want:

_coeffMat = loadCsv("DarkRate", conf, prof).getMatrix();
_coeff_a = _coeffMat[samples][0];
_coeff_b = _coeffMat[samples][1];
_coeff_c = _coeffMat[samples][2];

[otherorb]

Thanks Jesse,

I'll update my code with those snippets.

The next part that I'm not terribly comfortable with is how, within mro/apps/hical/main.cpp, to check for both ZeroDark and ZeroDarkRate being "activated" (!SkipModule()) and to exit with a user error if they're both active. If one or the other is skipped in the configuration file, the SkipModule flag basically just fills HiVectors with 0s, and we can just subtract both vectors during the calibration stage in main.ccp.

[jessemapel]

I'm not super familiar with the dbprofile API, I don't see a way to check if a profile exists. We could add it, but @KrisBecker may have some input on a better way.

[KrisBecker]

Hi @otherorb...

There are several options that come to mind. The easiest perhaps is to set a flag above the first occurring module, ZeroDark or ZeroDarkRate, set the flag to indicate if it was executed (i.e., SkipModel(prof) == false), then test the flag in the next occurring module code block if it is also not skipped. This should trigger an error and you can then throw your exception.

The other option is to directly test skip status for both models and test for conflict:

bool zdark = SkipModule(hiconf.getMatrixProfile("ZeroDark"));
bool zrate = SkipModule(hiconf.getMatrixProfile("ZeroDarkRate"));

if ( (zdark == zrate) && (true == zdark) ) {  // Doesn't matter which one you test
  // Throw your error....
}

[otherorb]

Thanks Kris,

I'm a little confused by this boolean logic in your example. Does SkipModule(hiconf.getMatrixProfile()) return True if the module is skipped or True if the module is not skipped?

I think it's okay if both modules are skipped (perhaps someone is trying to test the earlier modules, for example). It's just problematic if neither module is skipped.

[KrisBecker]

Hey @otherorb, SkipModule() returns true if the module is to be skipped. You will see in the code that the test used for each module is:

if ( !SkipModule(hiconf.getMatrixProfile()) ) {
  // Execute the module...
}

So in my previous post, the (zdark == zrate) tests if both are true or both are false. And, based upon this definition, the other test in the code is wrong. The other test should be (false == zdark) which indicates both are not skipped, which is not what you want, and should result in an error.

I think all other conditions are allowed, one or both can be skipped.

Sorry for the confusion.

[jessemapel]

@otherorb Can you reply here about the status of this work. Here's my main questions:

• What remains to be completed?

• Are there specific difficulties that you ran into and are not resolved?

• How can this be validated?

• Are there any automated tests written?

[otherorb]

• What remains to be completed?
  I have pushed the last bit of changes to my branch/fork. These are the things I'm unsure of for various reasons:

1. ADC settings were recently updated so my approach to pulling those values will need to be verified; I haven't been able to compile the most recent ISIS version, so I'm working in a bit of a vacuum on this one.
2. Verification that the code as pushed to my github even works; I've been unable to compile (even without my changes).
3. I did not write any unit or app tests.

• Are there specific difficulties that you ran into and are not resolved?
  Mostly, I've been unable to compile the most recent dev version from USGS without breaking my own environment, so I haven't been able to verify that this code as-written can be compiled.

• How can this be validated?
  I will send to Ken a Python notebook/program that runs the calibration and generates values. I'll include in that email instructions on how to (ideally) configure the hical.____.conf file to output the "same" or similar output files for comparison.

• Are there any automated tests written?
  No.

[jessemapel]

Okay, I've gotten the code compiling and what appears to be applying the new dark current correction, see my fork https://github.com/jessemapel/ISIS3/tree/ZeroDarkRate. Tomorrow I'm going to start testing this out and writing some simple automated tests. I have a few questions:

1. Zero Dark appears to be doing some filtering on the dark current correction values. Is this something that should be done in Zero Dark Rate or should it just use the exponential correction for each sample?
2. The ADC setting appears to be on the original label, but not on the cube label. I don't know if we'll be able to use it to select the appropriate calibration file without doing some more work. How critical are the different ADC settings on this?

[rbeyer]

2. The ADC setting appears to be on the original label, but not on the cube label. I don't know if we'll be able to use it to select the appropriate calibration file without doing some more work. How critical are the different ADC settings on this?

Didn't we fix this in #4290?

[jessemapel]

1. The ADC setting appears to be on the original label, but not on the cube label. I don't know if we'll be able to use it to select the appropriate calibration file without doing some more work. How critical are the different ADC settings on this?

Didn't we fix this in #4290?

Oh cool! Our hical test image still has -9999, so I'll just use a more up to date image for testing.

[otherorb]

I did not see a significant improvement in results when I filtered, but I wouldn't mind if we had the ability to have a filter that is (currently) turned off by default. A simple smoothing was what I tested; something like what Zero Dark uses.

[jessemapel]

Another question. Should ZeroDarkRate be used over ZeroDark by default? I've updated the code so that it uses ZeroDark with old config files, but I'm not sure if I should update the default config file to use ZeroDarkRate.

[otherorb]

I would leave ZeroDark as default until Ken and the HiROC people have a chance to determine whether it should be switched to the new behavior by default.