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

Implement feature to search jobs with a sp- and doc-integrated filter #332

Merged
merged 32 commits into from
Feb 10, 2021

Conversation

vishav1771
Copy link
Contributor

@vishav1771 vishav1771 commented Apr 29, 2020

Description

This patch enables the search for jobs with a statepoint- and document-
combined filter. To specify whether a key is a statepoint- or a
document-key use prefixes 'sp.' and 'doc.' respectively. No prefix is
equivalent to the 'sp.' prefix.

This patch is backwards compatible with the exception that the index
scheme was slightly modified.

Related to issue glotzerlab/signac-dashboard#25 .

Minimal example

This patch enables queries such as this one:

 >>> project.find_jobs({'foo': 0, 'doc.bar': True})
 signac.contrib.project.JobsCursor({'project': 'project', 'filter': '{'sp.foo': 0, 'doc.bar': True}'})
 >>> # equivalent to:
 >>> project.find_jobs({'foo': 0}, {'bar': True})
 signac.contrib.project.JobsCursor({'project': 'project', 'filter': '{'sp.foo': 0, 'doc.bar': True}'})

Equivalently on the command line:

$ signac find foo 0 doc.bar true

All keys without a prefix are automatically interpreted as having the sp. prefix.

Motivation and Context

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

Example for a changelog entry: Fix issue with launching rockets to the moon (#101, #212).

csadorf and others added 12 commits May 19, 2019 16:03
This patch enables the search for jobs with a statepoint- and document-
combined filter. To specify whether a key is a statepoint- or a
document-key use prefixes 'sp.' and 'doc.' respectively. No prefix is
equivalent to the 'sp.' prefix.

This patch is backwards compatible with the exception that the index
scheme was slightly modified.
To canonicalize all job-find filters at the JobsCursor stage.
@vishav1771
Copy link
Contributor Author

@csadorf @vyasr I have updated the PR according to the master.
I have some doubts about the warning. I think it should warn while setting the root key as doc in statepoint and sp in document. Please guide.

@vishav1771 vishav1771 changed the title Feature/integrated queries Implement feature to search jobs with a sp- and doc-integrated filter Apr 29, 2020
@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #332 (b81817c) into master (e1de895) will decrease coverage by 0.06%.
The diff coverage is 89.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
- Coverage   77.19%   77.12%   -0.07%     
==========================================
  Files          42       42              
  Lines        5770     5828      +58     
  Branches     1130     1151      +21     
==========================================
+ Hits         4454     4495      +41     
- Misses       1030     1047      +17     
  Partials      286      286              
Impacted Files Coverage Δ
signac/contrib/filterparse.py 84.53% <87.23%> (+9.93%) ⬆️
signac/contrib/project.py 85.58% <90.00%> (-1.39%) ⬇️
signac/contrib/import_export.py 77.89% <100.00%> (ø)
signac/contrib/linked_view.py 83.91% <100.00%> (ø)
signac/contrib/schema.py 95.48% <100.00%> (ø)
signac/__main__.py 71.89% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1de895...0941688. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Apr 29, 2020

@vishav1771 could you clarify what you mean? What warning are you referring to?

@csadorf
Copy link
Contributor

csadorf commented May 2, 2020

@vishav1771 I'm also not sure what you are referring to, could you clarify, please?

@vishav1771
Copy link
Contributor Author

Sorry for the misunderstanding. Actually, I was referring to the task defined in the PR #188

Add warning about the use of sp and doc as root-level keys in documents.

@csadorf
Copy link
Contributor

csadorf commented May 8, 2020

#188

The reasoning was that users should generally avoid those keys in both cases, because otherwise it is difficult to know what they are actually referring to. I don't think it's a huge issue, because if you use doc or sp as a key in either one of them, you could always refer to them as sp.doc: .. or sp.sp: ... etc. So it's more of an issue of creating misleading structures, but I think the best thing is to probably just ignore this for now.

@vishav1771
Copy link
Contributor Author

Okay, I understand 👍 .

tests/test_project.py Outdated Show resolved Hide resolved
@vishav1771
Copy link
Contributor Author

I've changed all the functions, but still, if there's anything missing please do tell!

@csadorf csadorf self-requested a review May 11, 2020 12:47
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

I have a few questions and suggestions.

tests/test_shell.py Show resolved Hide resolved
tests/test_project.py Outdated Show resolved Hide resolved
tests/test_project.py Outdated Show resolved Hide resolved
tests/test_project.py Outdated Show resolved Hide resolved
tests/test_project.py Outdated Show resolved Hide resolved
tests/test_shell.py Outdated Show resolved Hide resolved
@vyasr vyasr assigned vyasr and vishav1771 and unassigned vyasr Feb 2, 2021
@vyasr vyasr removed the blocked Dependent on something else label Feb 2, 2021
Copy link
Collaborator

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Looks pretty good, mostly just need a few more tests and I just had some minor points to improve clarity!

signac/contrib/filterparse.py Show resolved Hide resolved
signac/contrib/filterparse.py Show resolved Hide resolved
signac/contrib/filterparse.py Show resolved Hide resolved
signac/contrib/filterparse.py Show resolved Hide resolved
signac/contrib/filterparse.py Outdated Show resolved Hide resolved
signac/contrib/filterparse.py Show resolved Hide resolved
signac/contrib/filterparse.py Outdated Show resolved Hide resolved
signac/contrib/project.py Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Feb 2, 2021

@mikemhenry is going to work on adding a few more of these tests.

@mikemhenry mikemhenry mentioned this pull request Feb 9, 2021
12 tasks
@vyasr vyasr enabled auto-merge (squash) February 10, 2021 18:11
@vyasr vyasr disabled auto-merge February 10, 2021 18:11
@vyasr vyasr merged commit 3680496 into glotzerlab:master Feb 10, 2021
mikemhenry added a commit that referenced this pull request Feb 12, 2021
@bdice bdice mentioned this pull request Feb 12, 2021
mikemhenry added a commit that referenced this pull request Feb 12, 2021
vyasr added a commit that referenced this pull request Feb 12, 2021
vyasr added a commit that referenced this pull request Feb 12, 2021
…ntegrated filter (#332)" (#499)" (#501)

This reverts commit 27e782e, which was an undesired reversion.
@bdice bdice mentioned this pull request Feb 20, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expertise-needed Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify or improve consistency between groupby and groupbydoc
4 participants