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

1110 join single point error #1111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DLafayetteII
Copy link
Contributor

@DLafayetteII DLafayetteII commented Jan 4, 2023

Changes

Allowing join to succeed with data of shape (1,). Previously failed due to specific handling of data where np.ndim(data.variable) = 0. See issue #1110 for more info and the example failure case. Unsure of the motivation behind this specific handling of (1,) shape data, but this fix makes the join method work as expected.

Checklist

  • added tests, if applicable
  • updated documentation, if applicable
  • updated CHANGELOG.md
  • tests pass

@kameyer226
Copy link
Contributor

Code seems good, only move transpose.append statement in front of if...else since it is needed for both (all) cases, and remove the "no transpose needed" phrase

@kameyer226
Copy link
Contributor

Just a moment, a new issue may appear with this fix. What if the variable of shape (1,) has a different value for each of the arrays being joined? If they are different, the join should fail, shouldn't it?

@DLafayetteII
Copy link
Contributor Author

Just a moment, a new issue may appear with this fix. What if the variable of shape (1,) has a different value for each of the arrays being joined? If they are different, the join should fail, shouldn't it?

I am realizing that I have thus far only considered when the variable of shape (1,) is the transformed axis along which you are joining. In this case, it is fine if the values are different. If the shape (1,) variable is not your transformed axis along which you join, I want to say there's still no problem, but I have not tested and do not have a strong intuition.

@kameyer226
Copy link
Contributor

Yes, I should have stated the (1,) variable is the transformed axis along which to join. But why would it be fine if they are different? If they are different, isn't that saying they are not to be joined on top of each other?

@kameyer226
Copy link
Contributor

It looks good.

@ksunden
Copy link
Member

ksunden commented Jan 10, 2023

I'm a little confused as to how you got a scalar in the first place... I don't think we officially have supported such data objects, as their behavior is just different enough in all of these situations...

(Certainly you can have a size 1 data object with shape [1,1,1] for however many ndim you have, just thought scalar was not something we officially supported...)

@ksunden
Copy link
Member

ksunden commented Jan 10, 2023

note that scalar and shape (1,) are two different ideas, the latter will not have np.ndim(var) == 0

@ksunden
Copy link
Member

ksunden commented Jan 10, 2023

I would tend to want a test to show the failing behavior (and prevent it from recurring in the future... if we missed an edge case once, we could do so again)

@kameyer226
Copy link
Contributor

The variable was originally shaped (1,) prior to join, so it was never a scalar. I tried joining data objects consisting of scalar variables and the attempt failed.

@DLafayetteII
Copy link
Contributor Author

DLafayetteII commented Jan 11, 2023

note that scalar and shape (1,) are two different ideas, the latter will not have np.ndim(var) == 0

This misconception is the heart of the issue. We do np.ndim(var.points), and np.ndim(var.points)==0, so scalar and shape(1,) get treated the same in the code. Maybe we should change the code to do np.ndim(var) instead? @ksunden

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

Successfully merging this pull request may close these issues.

3 participants