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

Fixing DataArray.tracers for zero-sized data #1818

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

momchil-flex
Copy link
Collaborator

No description provided.

@momchil-flex momchil-flex requested a review from tylerflex July 11, 2024 09:16
@momchil-flex momchil-flex added the 2.7 will go into version 2.7.* label Jul 11, 2024
@tylerflex tylerflex requested a review from yaugenst-flex July 11, 2024 14:07
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Looks good but would like @yaugenst-flex to take a look since he wrote this part.

if AUTOGRAD_KEY not in self.attrs and not isbox(self.data.flat[0]): # no tracers
if self.data.size == 0:
return None
elif AUTOGRAD_KEY not in self.attrs and not isbox(self.data.flat[0]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be slightly faster to assign self.data.flat[0] to a variable to avoid doing this twice. but I dont know how expensive this operation really is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be very fast because it's just a view into the object I believe? But actually yeah this might be a good case for the walrus operator. Was avoiding it because I thought it was introduced way after 3.8... time flies.

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 if we need to involve the walrus if we can just assign a variable?

Copy link
Collaborator

@yaugenst-flex yaugenst-flex Jul 11, 2024

Choose a reason for hiding this comment

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

This seems pretty clean to me?

@property
def tracers(self) -> Box:
    if self.data.size == 0:
        return None
    elif not isbox(flat := self.data.flat[0]) and AUTOGRAD_KEY not in self.attrs:  # switched order of evaluation so flat is always assigned
        return None
    elif isbox(flat):
        return anp.array(self.values.tolist())
    else:
        return self.attrs[AUTOGRAD_KEY]

Compared to

@property
def tracers(self) -> Box:
    if self.data.size == 0:
        return None

    flat = self.data.flat[0]
    if AUTOGRAD_KEY not in self.attrs and not isbox(flat):
        return None
    elif isbox(flat):
        return anp.array(self.values.tolist())
    else:
        return self.attrs[AUTOGRAD_KEY]

Note that we need to add this level of nesting since self.data.flat[0] might error when self.data.size == 0. (edited because stupid)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You don't need else after return?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh man 😂 yeah

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

LGTM, I'm just wondering where this happened to cause a bug? I guess zero-sized data will always lead to an exception somewhere, not necessarily autograd-related?

@momchil-flex
Copy link
Collaborator Author

momchil-flex commented Jul 11, 2024

LGTM, I'm just wondering where this happened to cause a bug? I guess zero-sized data will always lead to an exception somewhere, not necessarily autograd-related?

There's a backend test that tests some edge cases. I think one way this could happen is if you have a TimeMonitor with a start time that is after the time at which the simulation reaches early shutoff. Then the simulation data contains an array with size on the t coordinate of 0. Before autograd, this used to pass. :)

@momchil-flex
Copy link
Collaborator Author

To recap should I be changing anything here or not? :)

@yaugenst-flex
Copy link
Collaborator

I think it's fine as-is. Don't think the flatiter will be a bottleneck 😄

@tylerflex
Copy link
Collaborator

agree

@momchil-flex momchil-flex merged commit 3bdb953 into develop Jul 11, 2024
4 of 16 checks passed
@momchil-flex momchil-flex deleted the momchil/data_array_tracers branch July 11, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 will go into version 2.7.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants