Eliminate xarray->numpy->xarray conversion in lo_l2 - #3371
Open
jaredclaypoole wants to merge 2 commits into
Open
Eliminate xarray->numpy->xarray conversion in lo_l2#3371jaredclaypoole wants to merge 2 commits into
jaredclaypoole wants to merge 2 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
Author
|
Okay, after bypassing some pre-commit weirdness I no longer have to use a |
tmplummer
reviewed
Aug 6, 2026
tmplummer
left a comment
Contributor
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) |
Contributor
There was a problem hiding this comment.
I think this can be simplified by calling xr.where directly:
Suggested change
| return (numerator / denominator.where(exposed)).where(exposed, 0) | |
| return xr.where(exposed, numerator / denominator, 0) |
| 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) |
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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