Catch nans before they cause crash in auto_ticks#636
Conversation
There was a problem hiding this comment.
LGTM. As usual, it'd be awesome if we could have a test for this regression but I tested this with the code from the issue - and this fix does prevent the plot window from crashing. Note that I still see the following on the command prompt when i click on the empty plot -
> edm run -e chaco-test-3.6-pyqt5 -- python .\chaco_bug.py
C:\Users\rporuri\work\github\ets\chaco\chaco\data_range_1d.py:276: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
if self._high_setting != val:
C:\Users\rporuri\work\github\ets\chaco\chaco\data_range_1d.py:283: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
if val == "auto":
C:\Users\rporuri\work\github\ets\chaco\chaco\data_range_1d.py:290: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
elif val == "track":
output of edm list for "chaco-test-3.6-pyqt5" edm environment |
| mapper = getattr(plot, direction + "_mapper") | ||
|
|
||
| # there is nowhere one could pan! | ||
| if mapper._null_screen_range or mapper._null_data_range: |
There was a problem hiding this comment.
This is where we start getting weird things (at least for the PanTool), so I try to catch it early. Unfortunately this trait is not defined on the AbstactMapper base class, so I am uncertain if this is safe here. However, it is defined on all of the LinearMapper, LogMapper and PolarMapper classes.
Ultimately we are ending up in a situation where newlow = [-inf], newhigh = [-inf], domain_min = -inf, domain_high = inf. I do not understand why LinearMapper.map_data returns a singleton array of the low end of the range when one of the null traits is true, see:
Lines 60 to 63 in fdd858a
This reminds me of #528 / #589
I feel like we should not change the types of the return value for a function for special cases, or if we do make it cleary documented that that is an edge case and handle it appropriately. What happens here is that array([-inf]) gets carried through and tried to be handled as if it were a normal float representing a a point in data space. But there is no range of data space at all in this scenario
Also there is this comment on AbstractMapper:
chaco/chaco/abstract_mapper.py
Line 19 in fdd858a
but
domain_limits very much appear to be being used here. (Not super important just pointing it out)
Overall I am unsure we want to use these traits here, but we definitely need some sort of edge nan, inf, None, etc case checking here and in other tools.
There was a problem hiding this comment.
Note, the change from the original commit of this PR seems to solve the reported issue, but I wanted to investigate up the chain to see where things were coming from and catch it earlier
Ah this is from a |
|
Okay at this point I think I am probably trying to do too much here... Note in my latest commit, I change However there is a separate This is getting a little orthogonal to what this PR aims to address. As I am writing this I am thinking I will actually undo these changes now and perhaps make them in a follow up PR... 🤔 |
aaronayres35
changed the title
rahulporuri
left a comment
rahulporuri
left a comment
There was a problem hiding this comment.
Still LGTM with a couple of minor comments
| @@ -242,6 +243,10 @@ def auto_ticks( | |||
| An array of tick mark locations. The first and last tick entries are the | |||
There was a problem hiding this comment.
nit - this is related but also tangential. the docstring says that this method returns an array - i am not sure if that means that we should strictly return an empty array.
this goes back to the general issue you mentioned of checking and enforcing return types across chaco. i think this can be an umbrella issue where we accumulate and fix issues that we discover.
There was a problem hiding this comment.
I saw this too, but then at the end of the method it appears to return just a list so an empty list is consistent.
Perhaps I should update the docstring instead? Or maybe they both really should be arrays?
There was a problem hiding this comment.
I think I am going to push this through for now, but this can be re-evaluated as part of #662
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
2 participants
fixes #529