Eliminate xarray->numpy->xarray conversion in lo_l2 - #3371
Eliminate xarray->numpy->xarray conversion in lo_l2#3371jaredclaypoole wants to merge 3 commits into
Conversation
* Also had to add mypy error suppression in an unrelated class definition in lo_l2.py, because mypy is set to ignore typing from out-of-module imports
|
Okay, after bypassing some pre-commit weirdness I no longer have to use a |
tmplummer
left a comment
There was a problem hiding this comment.
Overall, this looks like a good improvement. I'd like @vineetbansal to look at this too.
| out=np.zeros_like(exposure), | ||
| where=exposed, | ||
| ) | ||
| return (numerator / denominator.where(exposed)).where(exposed, 0) |
There was a problem hiding this comment.
I think this can be simplified by calling xr.where directly:
| return (numerator / denominator.where(exposed)).where(exposed, 0) | |
| return xr.where(exposed, numerator / denominator, 0) |
There was a problem hiding this comment.
Good point. And I think that's equivalent to the following, which is more similar to the rest of the code change (calling where on the DataArray directly).
return (numerator / denominator).where(exposed, 0)
I did the above in my latest commit.
| f"error bound; their systematic errors are left at zero." | ||
| ) | ||
| intensity_upper = _divide(count_rate, np.where(valid, gf_low, 1.0) * energy) | ||
| intensity_upper = _divide(count_rate, gf_low.where(valid, 1.0) * energy) |
There was a problem hiding this comment.
With the simplification of _divide it seems like this code might be easier to read by directly implementing the division using the where function here. What do you think. IMO, it would make it clearer that it is using the exposed mask. For exampe:
intensity_upper = xr.where(exposed, count_rate / (gf_low.where(valid, 1.0) * energy), 0)
Just a thought.
There was a problem hiding this comment.
If _divide were used just once, I'd completely agree. But since the same logic gets used 6 times, my preference would be to keep the helper. Feel free to push back.
|
@tmplummer I replied to your comments and pushed another commit. Let me know what you think. Also added @vineetbansal as a reviewer. (I think he's out until Monday, but we can wait until he's back.) |
Closes #3357
Change Summary
File changes
In
imap_processing/lo/l2/lo_l2.py:_calculate_rates_and_intensitiesdict[str, np.ndarray]todict[str, xr.DataArray]energy,geometric_factor,gf_low, andgf_higharrays_dividehelper to take, operate on, and return DataArrays instead of numpy arrays_build_map_datasetvariablesparameter type to match the output of_calculate_rates_and_intensitiesLoSpinAnglePointingSet#type: ignore[misc]to prevent a mypy error:imap_processing/lo/l2/lo_l2.py:441: error: Class cannot subclass "PointingSet" (has type "Any") [misc]strict = trueandfollow_imports = skipstrict = Trueprevents it from being subclassed