Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit and Pin down return types of methods across chaco #662

Open
aaronayres35 opened this issue Apr 13, 2021 · 5 comments
Open

Audit and Pin down return types of methods across chaco #662

aaronayres35 opened this issue Apr 13, 2021 · 5 comments

Comments

@aaronayres35
Copy link
Contributor

Problem Description
See comment: #636 (comment)

There are many places in chaco where we aren't super consistent about the type being returned by specific methods and this has led to a variety of bugs / issues such as:
#255, #272, #289, #550, (possibly) #542

Expected behavior:

The return types should be consistent as defined on the relevant interface or base class to prevent bugs in strange edge cases.

@aaronayres35
Copy link
Contributor Author

Also, the hittest method on BaseXYPlot returning tuples bothers me a bit

@aaronayres35
Copy link
Contributor Author

There exist map_data_array methods on all the Mapper classes, but they are not used anywhere in the code base. I imagine their original intention was precisely such a distinction, in that map_data takes float->float, and map_data_array array->array. But most map_data implementations are vectorized, aside from edge cases, so they can do array->array directly.

We should possibly remove the map_data_array methods? The majority of them currently just call self.map_data() anyway. If we keep them we should draw a line in the sand about what returns what. That I imagine could be a hard pill to try to force users to swallow...

As I am investigating this more I become more and more unsure of what the right way to go about this is.. 🤔

@aaronayres35
Copy link
Contributor Author

aaronayres35 commented May 5, 2021

Running list of special handling because of discrepancies on input/return types:

  • if isinstance(self.plot, Base2DPlot):
    # Base2DPlot.map_data takes an array of points, for some reason
    return self.plot.map_data([point])[0]
    elif isinstance(self.plot, BaseXYPlot):
    return self.plot.map_data(point, all_values=True)[:2]
  • chaco/chaco/base_xy_plot.py

    Lines 443 to 446 in 8065abb

    # transform x,y in a 1x2 array, which is the preferred format of
    # map_screen. this makes it robust against differences in
    # the map_screen methods of logmapper and linearmapper
    # when passed a scalar
  • chaco/chaco/color_bar.py

    Lines 156 to 157 in 8065abb

    # LogMapper.map_data() returns something unexpected if low==high,
    # so we'll handle that case here.
  • chaco/chaco/scatterplot.py

    Lines 341 to 343 in 3259e05

    # The rest of this was copied out of BaseXYPlot.
    # We can't just used BaseXYPlot.map_index because
    # it expect map_data to return a value, not a pair.
  • # We may need to make map_screen more flexible in the number of dimensions
    # it accepts for ths to work well.

@aaronayres35
Copy link
Contributor Author

ref: #486

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

No branches or pull requests

1 participant