MISR outputs are not properly set to fillvalues for night columns

[brhillman]

It appears that MISR_mean_ztop is not properly reset to the fillvalue R_UNDEF for night columns. In MISR_COLUMN() in MISR_simulator.F90, the following code computes the joint histogram for sunlit points, or else sets values to fillvalue (lines 266-287):

if (sunlit(j) .eq. 1) then
   < do stuff >
else
   MISR_cldarea(j)         = R_UNDEF
   MISR_mean_ztop(npoints) = R_UNDEF
endif

Note that MISR_cldarea appears to be set correctly, but MISR_mean_ztop is only set for the last index, and fq_MISR_TAU_v_CTH is excluded entirely. The following correction should give the desired behavior:

if (sunlit(j) .eq. 1) then
   < do stuff >
else
   MISR_cldarea(j)   = R_UNDEF
   MISR_mean_ztop(j) = R_UNDEF
   fq_MIST_TAU_v_CTH(j,:,:) = R_UNDEF
endif

[brhillman]

On closer inspection, it looks like changes are needed to MISR_SUBCOLUMN too, where only the dist_model_layertops variable is set to fillvalue for night columns:

    ! Fill dark scenes 
    do j=1,numMISRHgtBins
       where(sunlit .ne. 1) dist_model_layertops(1:npoints,j) = R_UNDEF
    enddo

Since this routine also returns box_MISR_ztop and tauOUT, it seems like these should be properly masked if nighttime as well, unless the desired behavior is to handle this masking at a higher level in the code, in which case the masking of dist_model_layertops could be omitted here for consistency.

[dustinswales]

@brhillman @RobertPincus
There are a couple thing to untangle here.
Firstly, the night-time masking of MISR_mean_ztop is clearly incorrect, at least to me.
MISR_mean_ztop(npoints) = R_UNDEF
should be replaced by
MISR_mean_ztop(j) = R_UNDEF
But... I looked at the COSP1 code, and this indexing issue is present there as well, see line 465 of https://github.com/CFMIP/COSPv1/blob/master/MISR_simulator/MISR_simulator.f.
It surely looks like a bug, but we should check with Roj. I don't have his gitHub info, but we should pull him just to confirm.

Secondly, regarding box_MISR_ztop and tauOUT. Both of these are treated the same in COSP1 as in COSP2. The only difference is that these fields are output from misr_subcolumn() and used as inputs later to misr_column(), whereas this was all encompassed in a single routine misr_simulator() in COSP1.

I propose we ask Roj if he agrees with you proposed changes. Then would you be kind enough to open a PR with your proposed changes?

[RobertPincus]

Dustin, this is great. I’ve cc’d Roj on this email but I’m confident your bug fix for the MISR simulator is correct: that MISR_mean_ztop(npoints) = R_UNDEF should be replaced with MISR_mean_ztop(j) = R_UNDEF. Where would you have @brhillman mask the box_MISR_ztop and tauOUT variables? He should set them to R_UNDEF in misr_subcolumn()? Will that work with the current implementation of misr_column()?

[brhillman]

I'll gladly put together the PR, once we have the desired behavior straight (i.e., at which level we want the masking to be done).

[RobertPincus]

I'll gladly put together the PR, once we have the desired behavior straight (i.e., at which level we want the masking to be done).

That’s what I’m asking @dustinswales to explain.

[dustinswales]

I'll gladly put together the PR, once we have the desired behavior straight (i.e., at which level we want the masking to be done).
That’s what I’m asking @dustinswales to explain.

@RobertPincus
In COSP2, both tauOUT and box_MISR_ztop are local variables in cosp_simulator(), whereas in COSP1 they were local variables within MISR_simulator(). They weren't masked in COSP1, so I didn't mask them in COSP2. I don't know if they need masking, I defer to Roj. They aren't part of the output, so I don't think they need be touched.

The only change necessary is in misr_column(), Ben's first suggested correction.

[RobertPincus]

@dustinswales Fair enough, in the current setup, but would this hold if some decided to use the MISR optical depths and cloud top heights in a different aggregation? Seem like there's an argument for masking as low down as possible?

[dustinswales]

@brhillman
I view the masking/aggregation as part of the retrieval handled in misr_column(). The only masking being done is for nighttime points, the only aggregation is over cloudy subcolumns. If we do the masking higher up the code it won't have any impact on the aggregation in the column simulator later on.

The question I have is why are we calling the MISR_subcolumn() simulator for all columns if we are only using the daylit points in the statistics ? Seems like a waste, we could only process the sunlit points, compute the same stats, maybe save some time?

[brhillman]

Hey @dustinswales yeah I agree masking makes sense at the low-level (misr_column). To answer your question about useless calculations, right now misr_subcolumn loops over both columns and subcolumns. So as the code currently stands the easiest thing to do would be to add a logical check at each column to see if the column is night, and if so then bypass the calculations and instead set the column to R_UNDEF (right now it calculates for each column, and then fills after the fact). The other option would be to take the loop over columns out of misr_subcolumn, so inputs would only have a subcolumn dimension and no column dimension, and instead handle the loop over columns at the higher misr_column subroutine, only calling misr_subcolumn for sunlit columns. I don't think there will be a performance difference either way between these options (same amount of work) so I think it's up to you how you want the code structured.

[dustinswales]

@brhillman
Both of your suggestions would work, personally I prefer your first suggestion of adding a logical check in the MISR simulator code, similar to the ISCCP way, instead of the MODIS way (there's a bunch of extra code in cosp_simulator() to support the masking for MODIS nighttime scenes which may be better suited within the simulator)

[RobertPincus]

@dustinswales @brhillman I still can't wrap my head around apply the night-time filter at the column rather than the sub-column level, since there's no way for any passive instrument including MISR to make any retrievals at night. Is it just technically easier at this stage?

@dustinswales Should we open another issue in clean up the MODIS code to do better night-time filtering?

[dustinswales]

@RobertPincus
I propose we add masking over nighttime points in misr_subcolumn(), and add this to the simulator code (ISCCP-like). Just a simple logical switch that doesn't process subcolumn dark scenes. We can assign dark columns to fill at this point, but it won't matter as we use the sunlit mask later for the column aggregation. Of course there are other column fields that should be set to fill (Ben's OG post and suggestion)

Also. Yes let's open a corresponding PR to clean up the MODIS code to also handle the masking internally. I can do this.

[RobertPincus]

@dustinswales I like this idea. @brhillman, it's clear what's being proposed, and you can make a PR?

[dustinswales]

@brhillman
Ha. Now I remember the reason for this confusing piece of code you identified: #31
It's a hack for the MODIS simulator not internally masking the nighttime columns.

[RobertPincus]

@brhillman No waving my dirty laundry around in public, now. But yes...

It's great having you go through the code. You find all the warts.

[brhillman]

@RobertPincus @dustinswales yup I think it's clear, I'll put it together tomorrow.