Skip to content

Conversation

fabiansinz
Copy link
Contributor

  • relation.fetch.order_by allows for individual sorting arguments now. I removed the kwarg descending. The new syntax is relation.fetch.order_by('name DESC', 'trial', 'stimulus DESC'). If the sorting argument is omitted, ASC is set by default.
  • I removed the relation.fetch.apply function. It is not needed at the moment and simplified the syntax substantially. Additionally, it might cause confusion, since something like relation.fetch.apply(myfunc).order_by('stimulus')['name'] suggests that the function is applied in the beginning, but it would have been applied at the end.
  • I renamed FetchQuery to Fetch and Fetch1Query to Fetch1
  • I renamed limit_by to limit_to. Dimitri actually prefers limit, but I think limit_to is more consistent with order_by.
  • Wrote quite a few more tests.

@dimitri-yatsenko
Copy link
Member

note that order_by can operate on expressions, not just attribute names. For example, if I wish to sort so that name='Fabian' comes before all others I would need to say, according to the proposed syntax:

relation.fetch.order_by('name="Fabian"=DESC', 'trial')

This does not seem like good syntax. Plus the word DESC is hardly PEP8-friendly.

How about tuples ('expression', True) for descending.

relation.fetch.order_by(('name', True), 'trial', ('stimulus', True))

Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for copy_first?

Copy link
Contributor

Choose a reason for hiding this comment

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

We thought that it would make more sense for each operation on the fetch object to return a new fetch object rather than modifying the object in place. Since fetch object in itself is very cheap, this shouldn't be a real over head. copy_first is a decorator that modifies the decorated method to work on and modify the copy of the object, supporting this idea.

Copy link
Member

Choose a reason for hiding this comment

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

why not use copy.copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy.copy does not copy the dict behavior. The function deepcopy would copy _relation as well which seemed like an overkill. That's why I introduced a copy constructor and used a decorator to tell the function, that I'd like self copied. Having the elements of behavior as object attributes wouldn't solve the problem, since some of the values of behavior are lists.

Copy link
Member

Choose a reason for hiding this comment

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

But why do we need to copy the object in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

ok, i have looked into other modules and, yes, things like these usually copy the object. I am merging this PR.

dimitri-yatsenko added a commit that referenced this pull request Jul 25, 2015
Extended order_by and bugfixes
@dimitri-yatsenko dimitri-yatsenko merged commit dcb37ab into datajoint:master Jul 25, 2015
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.

3 participants