-
Notifications
You must be signed in to change notification settings - Fork 1
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
Search API: Retain nesting of queries, Data API: add querying for all structures #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @ivana-truong! This is looking excellent.
While you're still implementing the workaround for nested attribute issue based on our conversations, I thought I'd go ahead and leave a couple comments and suggestions on the querying all structures functionality.
1d77832
to
e9d742b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ivana-truong! These are wonderful enhancements, and very nice work on all of them. I just had a couple of suggestions and questions, but overall things look excellent.
Co-authored-by: Dennis Piehl <[email protected]>
e195430
to
0aa606b
Compare
0aa606b
to
5f98dea
Compare
This is a really nice solution! I don't know this package well, but would it be possible to flatten immediately before the query is run rather than greedily per If I understand correctly, the problem is that As an alternative to Sketch of binary tree solution# So a Query method can return Node:
from __future__ import annotations
from dataclasses import dataclass
@dataclass(frozen=True, slots=True)
class Node(SearchQuery):
"""Implementation as a tree."
def exec(self, ...) -> Union["Session", int]:
prepped: Node = self._optimize()
... # TODO
def _optimize(self) -> Node:
... # TODO: Traverse the tree, flatten compatible terms, etc.
@dataclass(frozen=True, slots=True)
class Group(Node):
operator: TAndOr
left: Node
right: Node
# Omit `keep_nested`.
@dataclass(frozen=True, slots=True)
class Conjunction(Branch):
def _optimize(self) -> Node: ... # TODO
@dataclass(frozen=True, slots=True)
class Disjunction(Branch):
def _optimize(self) -> Node: ... # TODO
# Other subclasses By the way: I really like the use of |
Thanks for the input and idea @dmyersturnbull! A binary tree would provide a nice data structure for managing this query construction. Will definitely keep this in mind as an option if we encounter other query-building issues in the future. The specific issue we were addressing here though wasn't so much with the package's ability to "flatten correctly"—in fact, it was actually flattening entirely as expected given the set of nodes and operators provided (as you phrased it, indeed "greedily"). Rather, the issue here was trying to prevent it from flattening when you want a specific set of nodes to be in the same group, even if it's logically the same as being flattened out. This is largely a consequence of relying on So basically this PR provides a way to avoid flattening out a subgroup, so while |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wonderful @ivana-truong! Thank you again for the excellent work! I just had two minor comments, but once you address those feel free to merge!
Search API
__and__
/__or__
method ofSearchQuery
and addedgroup
method in response to Issue in grouped attribute search #49. Now building queries withgroup
function will cause the group to be preserved while constructing the rest of the queryData API
const.py
/data.__init__.py
. This can be passed into theinput_ids
parameter to make a query for all structures. Currently supportsentries
andchem_comps
input_type
sprogress_bar
parameter to.exec
. If set toTrue
, there will be a progress bar for the executing query.batch_size
parameter to.exec
. Defaults to 5,000.Misc