Improve error messages in uxarray/grid - #1705
Open
Sevans711 wants to merge 2 commits into
Open
Conversation
(numba complains when trying to provide helpful info in error messages, such as including the value which was actually received. So, make those error messages worse, so that numba will be happy again. Left inline comments to help clarify this.)
Sevans711
added
documentation
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
Collaborator
Author
|
@cmdupuis3 Do you have any guesses about whether the increased peakmem values for face_bounds are likely to be real performance hits, or just an artifact of how the benchmarks were run? Similar question for the slowdown of I don't see any obvious routes in the codebase where these changes would have any significant impact on memory usage. (I would only really expect that to occur if the error message is being generated as part of the standard control flow, something like Less relevant to this PR, but also, do you have any guesses about why the bot edited the message 5 times after posting it? (Is there any change it is related to the benchmarking suite taking roughly 6 times longer than it took a few weeks/days(?) ago?) |
Collaborator
|
@Sevans711 I've been seeing some odd behaviors from the FaceBounds benchmarks lately. I'm rerunning it to see if we get the same 38k -> 50k behavior in a second round |
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 #1704 (sub-issue of #1617)
Overview
Makes error messages clearer and more informative throughout uxarray/grid. As suggested by #1617 this is attempting to be an exhaustive fix for all desired error message improvements in uxarray/grid, while touching nothing outside of uxarray/grid. (Clarifying, this is for all files in uxarray/grid, not just files inside uxarray/grid/grid.)
Small expansion of scope: fixes confusing/misleading error types in a few places:
insert_pt_in_latlonbox: "Logic error in longitude box width calculation". (Missed this while fixing Update uxarray error types for cases with misleading/confusing types #1622, which did mention this case but it actually appeared twice and the fix missed the second occurrence.)Grid.isel()messages like "Inverse indices are not yet supported for node selection, please use face centers". DataCenteringError was also misleading because these errors depended only on input kwargs, not the actual location of any data._slice_face_indices: "Exclusive slicing is not yet supported." (Missed this while fixing Update uxarray error types for cases with misleading/confusing types #1622 despite it being noted there explicitly.)Misc. small expansions of PR scope:
_validate_minimum_ugridcheck which was directly above it (which will crash when invalid). I suspect that for a while only the warning existed, and then whatever PR added the check forgot to also remove the warning.Grid.from_face_verticesto clarify expected shape and content of face_vertices, and the meaning of latlon=False._compute_face_areas_and_jacobian: actually utilizemin_jacobian(computed asnp.min(self._face_jacobian)), by checkingmin_jacobian < 0, instead of computing it then ignoring it to instead checknp.any(self._face_jacobian < 0). Additionally, do not computemax_jacobiananymore; previously it was always being computed despite only being used if raising an error, and now it is no longer included in the error message at all.Aside from these small expansions of scope there should be no functionality changes; the only differences in this PR are the changes to error message contents.
Benchmarks: running benchmarks to make sure there isn't any performance hit. (Highly unlikely, but could happen if errors are used as part of control flow in a hot loop and any objects now placed into error message strings take non-negligible time to convert to string.)
PR Checklist
General
Testing & Benchmarking
AI Disclosure
AI Usage: GitHub Copilot's inline code suggestions, some small chats with Claude