Add CALIOP level2 extinction coefficient converter - #1786
Conversation
|
@weiwilliam is there a public facing archive of this data |
BenjaminRuston
left a comment
There was a problem hiding this comment.
@weiwilliam this looks good from a high level view will see if we can't get some others to review this as well
|
@mer-a-o or @jeromebarre do you have any concerns with this contribution from @weiwilliam we can always do small clean-up and changes moving forward. Is there anyone else who we should add to review? @weiwilliam where are public facing files to test with? |
|
@jeromebarre here is the converter... @weiwilliam I don't see is this correct |
|
@BenjaminRuston Yes, but that is because there are minor differences between what they specified and the calculation. Above 20.2 km, they specified 180 m, while the calculation is ~179.62 m. Below 20.2 km, they specified 60 m, while the calculation is ~59.87 m. |
|
@weiwilliam what I understood was the thickness was a value in the raw data itself already calculated you are explaining the height is regular and changes between values at a threshold, and yes if it's computed maybe we only need the at this point ... if both are passed in and used in UFO there is a chance of misuse, while if only so the recommendation I believe is to remove the thickness from the converter, only require |
|
@BenjaminRuston Can you give me a example, in what case, passing both |
|
you've got me really confused @weiwilliam but maybe we should talk but the point was if another datatype comes along simplifying what is needed to the truly required parameters of the operator we need approvals here and consensus so lets get that done |
|
@BenjaminRuston Have you checked my response https://github.com/JCSDA-internal/ufo/pull/4163#discussion_r3633860077? |
| thickness[-1] = height[-2] - height[-1] | ||
|
|
||
| # Adjust thickness near 20.2 km because it should be around 180m above and 60m below 20.2km | ||
| tmpidx = np.argmin(np.abs(height-20200)) |
There was a problem hiding this comment.
The 20200 breakpoint is hardcoded with no assertion that the grid is the standard APro grid, and np.abs(...) in the patch relies on the blended value always sitting between the two true thicknesses. That holds for this grid but fails silently on any other. Worth an explicit check on the altitude array before applying the correction. Relatedly, the grid is read only from self.filenames[0] and assumed identical across all inputs.
There was a problem hiding this comment.
@BenjaminTJohnson Yes, it is very specific to the APro product only, maybe to the CPro product and other Level 2 products as well. And yes, it is identical across all files.
This converter is only working for the Level 2 APro product. I will see if I can find any way to safeguard it. The product ID is "L2_LIDAR", which is also used for CPro product.
| } | ||
|
|
||
| VarDims = { | ||
| 'extinctionCoefficient_532nm': ['Location', 'Channel'], |
There was a problem hiding this comment.
Using Channel as the vertical dimension while folding the two actual wavelengths into variable names (extinctionCoefficient_532nm, extinctionCoefficient_1064nm) inverts the usual IODA convention. It works, but it is the kind of thing that is painful to change once downstream UFO configs depend on it, and it is worth settling now rather than after merge. Also, obs values are in km⁻¹ while all heights are in m, which is a mixed-unit file.
There was a problem hiding this comment.
The ideal dimension is (Location, Channel, Layer), however, it is not achievable.
It is a workaround to meet current capabilities in IODA ObsSpace. The IODA ObsSpace has to be (Location, Channel). With https://github.com/JCSDA-internal/ioda/pull/1764, it can be (Location, Layer) but we still cannot bypass that hofx has to be 2D.
I do consider converting km-1 to m-1. Thanks for bringing this up.
There was a problem hiding this comment.
Merging https://github.com/JCSDA-internal/ioda/pull/1764 doesn't prevent using (Location, Layer) right now.
https://github.com/JCSDA-internal/ioda/pull/1764 allows to use filters on 2D arrays that are (Location, Layer) on top of (Location, Channel) that's all. For this group of PRs for Caliop and profile extinction operator I don't htink there is anything that prevents using Layer instead of Channels and that would be more appropriate.
Description
This PR adds the converter for CALIOP APro level 2 extinction coefficient.
Issue(s) addressed
Resolves #1711
Dependencies
N/A
Impact
Expected impact on downstream repositories:
Manual Testing Instructions (optional)
If you would like your reviewers to manually build and test the change, please include
instructions on how the change should be built and tested. Also include a short
justification on why manual testing is necessary for this change.
Checklist