Skip to content

Fix some scatter plot issues #7167

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

Merged
merged 6 commits into from
Oct 17, 2022
Merged

Fix some scatter plot issues #7167

merged 6 commits into from
Oct 17, 2022

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Oct 16, 2022

Fix some issues with scatter plots:

  • Always use markersize widths for scatter.
  • Fix issue with .values_unique not returning the same values as .values
  • Added more type hints to _Normalize, some rework had to be done to make mypy pass.

xref: #6778

@headtr1ck
Copy link
Collaborator

Unrelated to this PR but I noticed that typing of _Normalze was quite difficult because _unique_inverse can be a np.ndarray or DataArray, maybe you could clear that up a bit?

@Illviljan
Copy link
Contributor Author

I've reworked _Normalize a little now. Hopefully a little easier to follow. Still not a fan of the optional None outputs, it adds too much extra code. Oh well, will save that for another rainy day.

@Illviljan Illviljan marked this pull request as ready for review October 16, 2022 15:41
@Illviljan Illviljan changed the title Fix some scatter issues Fix some scatter plot issues Oct 16, 2022
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, the optional return make stuff a bit complicated. Not sure how to solve this though?
Maybe an extra class for the case of data=None?
Or it could be solved with a Generic class?
But I agree that this can be solved in another PR.

Thanks for addressing this!

def _indexes_centered(self, x: DataArray) -> DataArray:
...

def _indexes_centered(self, x: np.ndarray | DataArray) -> np.ndarray | DataArray:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you didn't like the TypeVar here? But ok, the results are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mypy started complaining when I added typing to .values, so I tried Union and finally overloads. This was one of the earliest changes though so maybe it works now when I changed ._unique_inverse ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe one of these instances where you need to use a TypeVar bound to a Union instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, I'll tests this a bit more later once I find a new bug. This works good enough for now.

@headtr1ck headtr1ck added the plan to merge Final call for comments label Oct 16, 2022
@Illviljan Illviljan merged commit 0aa0ae4 into pydata:main Oct 17, 2022
dcherian added a commit to shoyer/xarray that referenced this pull request Oct 17, 2022
* main:
  Add import ASV benchmark (pydata#7176)
  Rework docs about scatter plots (pydata#7169)
  Fix some scatter plot issues (pydata#7167)
  Fix doctest warnings, enable errors in CI (pydata#7166)
  fix broken test (pydata#7168)
  Add typing to plot methods (pydata#7052)
  Fix warning in doctest (pydata#7165)
  dev whats-new (pydata#7161)
  v2022.10.0 whats-new (pydata#7160)
dcherian added a commit to JessicaS11/xarray that referenced this pull request Oct 17, 2022
* main:
  Add import ASV benchmark (pydata#7176)
  Rework docs about scatter plots (pydata#7169)
  Fix some scatter plot issues (pydata#7167)
  Fix doctest warnings, enable errors in CI (pydata#7166)
  fix broken test (pydata#7168)
  Add typing to plot methods (pydata#7052)
  Fix warning in doctest (pydata#7165)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants