Corrections for Lo Maps - #3370
Conversation
tmplummer
left a comment
tmplummer
left a comment
There was a problem hiding this comment.
This is a huge diff to look at. I did my best. Mostly this code looks really good! My main gripe is that the algorithms that I implemented based off of Nathan's alg docs are no longer getting used for Lo. I will note that those algorithms were validated using Nathan's data so at some point it would be good to understand if he changed something or if they could be pulled in and used.
| corrected = counts - np.einsum("ts,esp->etp", sputter_matrix, source_counts) | ||
| variance = counts + np.einsum("ts,esp->etp", sputter_matrix**2, source_counts) |
There was a problem hiding this comment.
einsum is great to use, but can you try to describe what these are doing?
| return speed**2 * energy_u | ||
|
|
||
|
|
||
| def _compton_getting_correct_intensity( |
There was a problem hiding this comment.
Did Nathan totally change the CG correction. The common CG correction code that I wrote was based off of his document so I would have thought that it could be used on Lo data. The part that adjusts PSET data is here: https://github.com/IMAP-Science-Operations-Center/imap_processing/blob/dev/imap_processing/ena_maps/utils/corrections.py#L736
Then the interpolation to the helo-frame energies is here: https://github.com/IMAP-Science-Operations-Center/imap_processing/blob/dev/imap_processing/ena_maps/utils/corrections.py#L797
| change = np.sqrt(np.nanmean((source / previous) ** 2)) - 1.0 | ||
| if np.isfinite(change) and abs(change) < c.CG_CONVERGENCE_TOLERANCE: | ||
| logger.debug(f"Source spectrum converged after {iteration + 1} iterations") | ||
| break |
There was a problem hiding this comment.
This seems very close to what the PowerLawFluxCorrector does. Is it not usable? I wrote it and validated it based on Nathan's algorithm document.
There was a problem hiding this comment.
It seems to me like the algorithm document may have diverged since the code was first written. Specifically, I'm seeing:
- The current implementation of
estimate_power_law_slopeinPowerLawFluxCorrectoruses central differences, while the algorithm document (and this implementation) uses forward differences. (eq A1) - The top level uses backward difference in this implementation, while in the algorithm document (and this implementation) we copy the level below. (eq A2)
I suppose its possible to emulate the new behavior by adding optional flags to the function. Maybe we open an issue on this and get back to it?
There was a problem hiding this comment.
If it has diverged, it might make sense to not force it into shared code. Originally, Hi and Lo had different algorithms for this stuff specified. The project leadership said they needed to use the same algorithm, so Hi adopted Nathan's algorithm but were not too happy about it. So, at this point, maybe letting Hi and Lo diverge makes sense and that way either team can modify at will without any side-effects.
| CG_MAX_ITERATIONS: int = 20 | ||
| CG_CONVERGENCE_TOLERANCE: float = 0.005 |
There was a problem hiding this comment.
This is a really big PR, so I didn't track it down, but IIRC, the flux correction is totally separate from the CG correction. Doesn't the flux correction get applied to maps in either the s/c or helio-frame? I would expect these variables to be specifically named to flux correction rather than having to do with the CG correction.
| # in the spacecraft frame purely from the spacecraft's own motion, i.e. | ||
| # 1/2 m_H U^2 at the nominal spacecraft speed of ~30 km/s. The kinematics | ||
| # of the correction are scaled by it. | ||
| CG_ENA_ENERGY_AT_SPACECRAFT_SPEED_EV: float = 4.661 |
There was a problem hiding this comment.
In the CG correction that I implemented, this was computed exactly using SPICE. Probably is just in the noise.
There was a problem hiding this comment.
yes - spice is the correct way to go. Almost none of the steps in the dropbox use spice, and I didn't realize you'd already implemented this. I'll look at your implementation and use it instead.
|
Thanks for taking the time to review @tmplummer - I'll go through your suggestions and reduce code duplication where I can. Perhaps we can create issues on the ones I'm not able to tackle right away and I'll get to those asap. The major change in Lo's approach seems to be that equations were written for a pre-binning stage (psets), but later on they moved to a map-making approach that creates bins first, and then modifies intensities for each bin through these corrections. It may be possible to leverage existing code to do so, but it will take some more investigating. I'm out for a couple of days but will pick this up again next Monday. |
tmplummer
left a comment
tmplummer
left a comment
There was a problem hiding this comment.
I'm fine with these changes. As you noted, any places that use of shared code can be investigated are fine to capture in new issues.
2 participants
Added sputter/bootstrap/CG correction, and ISN masking for Lo maps.
The combined maps after ISN-masking are likely incorrect, since I still don't know how that masking is supposed to get done for combined maps (currently a pixel of the combined map is masked if any of those pivots would have masked it.)