Skip to content

Fix outliers() crash on bounded parameters with string dimension limits - #61

Closed
arpitjain099 wants to merge 1 commit into
DOI-USGS:developmentfrom
arpitjain099:fix/outliers-string-bound-crash
Closed

Fix outliers() crash on bounded parameters with string dimension limits#61
arpitjain099 wants to merge 1 commit into
DOI-USGS:developmentfrom
arpitjain099:fix/outliers-string-bound-crash

Conversation

@arpitjain099

Copy link
Copy Markdown

Hi, thanks for maintaining pyPRMS.

Parameter.outliers() raises for any bounded parameter. The metadata loader keeps a dimension name as the maximum (for example nlake on lake_hru_id) when it can't cast the bound to the parameter datatype, so outliers() ends up doing a numpy > between the int data and a string and hits a _UFuncNoLoopError. It affects the whole set of bounded params (lake_hru_id, hru_solsta, the cascade _down_id/_up_id parameters, and more).

check_values() already skips the comparison when a bound is a string, so this just brings outliers() in line with that. Numeric bounds behave exactly as before; string bounds are skipped and no longer counted.

Added a case to test_Parameter covering the bounded parameter.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@pnorton-usgs

Copy link
Copy Markdown
Member

Hi Arpit,

Thank you for your PR on the outliers() crash with bounded parameters. As it turns out this was indication of a greater problem in the code. The intent for bounded parameters was that the minimum and maximum metadata values would be changed to integer values when a bounded parameter was added reflecting the full range of acceptable values for the parameter. In practice this was not happening in all cases. This problem was addressed in PR #63 and #65 so bounded parameters should now be fully resolved when they are added and the outliers() function should work properly with them. Currently poi_gage_id is the only true string parameter where the minimum and maximum metadata values would be strings; so when a parameter is a string the outliers() code will return an empty list. Non-string parameters should raise an error if minimum or maximum metadata is a string.

After applying fixes the changes you have suggested no longer apply. Thank you for your help in uncovering this problem. I will close this PR at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants