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

adds reshaping functions to Images and Series #343

Merged
merged 2 commits into from
Aug 2, 2016

Conversation

jwittenbach
Copy link
Contributor

implements #335 for Series and a corresponding function for Images

@jwittenbach
Copy link
Contributor Author

@freeman-lab: I want to use this to make the handling of non-2D arrays nice and straight-forward in Thunder Factorization. Seemed general enough to include directly in Thunder. Thoughts?

@freeman-lab
Copy link
Member

Cool, seems reasonable, is this the only case where the thunder API directly references keys and values? That's the only thing that concerns me a bit. If we've avoided that so far, might not want to introduce it just for this. Other options, off the top of my head:

  • You could call both reshape but it's confusing if they do different things
  • You could only add a reshape method to series, because for images you can always do reshaping via a map operation, but that doesn't work for "reshaping" the keys in series data

Other ideas?

@jwittenbach
Copy link
Contributor Author

Yeah, I also thought about just calling it reshape, but decided it was too confusing. We do mention "values" in a couple of places (e.g. value_shape), but there is no equivalent for "keys", which makes things tricky.

raise ValueError("Total number of series elements must remain unchanged")

newshape = shape + (self.shape[-1], )
return self._constructor(self.values.reshape(newshape)).__finalize__(self)
Copy link
Member

@freeman-lab freeman-lab Aug 1, 2016

Choose a reason for hiding this comment

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

should be self.keys.reshape right?

Copy link
Member

Choose a reason for hiding this comment

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

oh sorry, nevermind, this is correct, i was confused

@jwittenbach
Copy link
Contributor Author

@freeman-lab updated things to reflect our conversation yesterday...both Images and Series now have a reshape function with a simpler API that just takes the new shape, and then catches invalid reshapes.

@freeman-lab
Copy link
Member

Nice @jwittenbach , this looks great! Merging in.

@freeman-lab freeman-lab merged commit 18ce991 into thunder-project:master Aug 2, 2016
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

Successfully merging this pull request may close these issues.

None yet

2 participants