-
Notifications
You must be signed in to change notification settings - Fork 42
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
Long distance restrictions #949
Conversation
key_from_upstream
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.
One required bug fix, and a few usability ideas.
One other more general idea:
- From testing, it seems like a multi-part restriction only works through this if all the keys are found in the same up(down)stream table
- I assume the correct way for a user to do this would be call
restrict_by
separately for each key and then combine the results. - Could be nice to happen inside the restrict_by function (e.g. split the restriction into parts, call
restrict_by
for each, then return the intersection fo the results
return graph | ||
|
||
ret = graph.leaf_ft[0] | ||
if len(ret) == len(self) or len(ret) == 0: |
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.
A usability thought/suggestion. As you mentioned, the shortest path option can get tricky with merge tables. In testing this I would end up manually repeating the call when it went the "wrong" way with no results and adding to the banned list each time.
When an empty restriction set is found would it make sense to add whatever the top of the cahin was to the banned list and then recursively call restrict_by
?
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.
The stop condition would either be finding a non-empty set or hitting the error where the key is no longer in the graph
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.
My plan for the immediate future is to get a sense of how useful it is from folks. If useful, if the table banning process is a chore, I would continue the TableChain
search until all tables in the path had a valid restriction. Rather than deciding the path, I would depth-first search until there was a valid restriction the whole way.
It seemed like a lot of work to put in without that initial feedback, but I can revisit if you think it's needed
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.
That's fair. I agree it's not required for this PR.
As another option that doesn't require action right now; maybe a more efficient solution would be something like a required_tables_list
where a chain is only valid if it contains the tables in that list. Would potentially solve the case of picking from a certain source of a merge table.
Co-authored-by: Samuel Bray <sam.bray@ucsf.edu>
That's true - I had assumed that the passed restriction was valid on exactly one table elsewhere. If the user wanted to restrict by multiple, I think they could |
I was trying something like This is another that was just me trying to test it as a naive user so it's fair for it not all implemented at release if the core function is working. |
Across these cases, (1) uses additional info from the user that these likely come from different tables, and implementing (2) would assume they do not, which incurs the time cost of checking each table to see where they belong. I'd consider that time cost an annoyance, but maybe the average user wouldn't? Worth asking when I collect feedback |
Description
This PR refactors
dj_chains
anddj_graph
to use centralized logic for navigating graphs. See new docs for new notation using<<
and>>
operators.Other minor changes
camel_name
as a Mixin propertyfuzzy_get
to dj_helper to permit indexing of custom objectsPERIPHERAL_TABLES
to dj_helper to avoid circular importimport_merge_table
backup based on user feedback tosearch_descendants
Checklist:
CITATION.cff
alter
snippet for release notes.CHANGELOG.md
with PR number and description.