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.

Sign up for GitHub

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

Jump to bottom

test: add unit tests and refactor filter computer notebooks #682

Closed
wants to merge 38 commits into from
Closed

test: add unit tests and refactor filter computer notebooks #682

wants to merge 38 commits into from

Conversation

danieljhegeman
Copy link

@danieljhegeman danieljhegeman commented Nov 9, 2023 •
edited
Loading

Reason for Change

Changes

  • refactor logic
  • add unit tests
  • add type hinting
  • improve var naming

Testing

  • Either list QA steps or reasoning you feel QA is unnecessary
  • Reminder For CLI changes: upon merge, contact Lattice for final sign-off. Do not release a new cellxgene-schema
    version to PyPI without explicit QA + sign-off from Lattice on all functional CLI changes. They may install the package
    version at HEAD of main with
pip install git+https://github.com/chanzuckerberg/single-cell-curation/@main#subdirectory=cellxgene_schema_cli

Notes for Reviewer

@codecov Codecov
Copy link

codecov bot commented Nov 9, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5260fcb) 84.68% compared to head (785d6a5) 84.68%.
Report is 17 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #682   +/-   ##
=======================================
  Coverage   84.68%   84.68%           
=======================================
  Files          19       19           
  Lines        1815     1815           
=======================================
  Hits         1537     1537           
  Misses        278      278
Flag Coverage Δ
unittests 84.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Daniel Hegeman added 28 commits November 14, 2023 14:04
fix: var name typo -> entity_name
e0f2281
refactor pygraphviz.AGraph import, add some type hints
6e23ff2
more type hints
a1aeb52
fix: child_visited var typo
bd6e80e
var name change child_visited -> child_node_exists
606a2b0
More type hinting, add comments to clarify node/edge additions
df4e853
more type hinting, remove superfluous return in build_ancestor_set
ca3c6a1
more type hinting, fix var name for list
486b88e
rename list_descendants -> build_descendants_set
c5fadc7
more cleanup
374edab
minor comment
6c64e68
fix descendant -> ancestor var name in key_ancestors_by_entity
89e2e5e
remaining type hints
ed13ea8
init test and reorg script to only execute graph build on main
612d01a
add tests for building descendants graphs
de812be 
- tissues
- cells
test: build ancestor set
e333353
test: build descendants set
df8db98
test: write_ancestors_by_entity
d1958b6
refactor: file writes to make code testable without creating empty files
506192b
test: write_descendants_by_entity
1fcaad4
deal with mypy
5997560
rename ancestor -> ancestors where appropriate
aa3a1ad
rename sanitized -> formatted
55f894c
rename ancestor -> ancestors in test file
6d4d86c
make graph directed
8680a09
followup - missed an ancestor var name
adcdc24
test: organoid descendant/ancestor tracing termination
d32f246 
Add organoid with both ancestor and descendant to test feature where
ancestor and descendant tracing stops whenever an organoid is
encountered.
minor refactor
296b44b
@danieljhegeman danieljhegeman changed the title docs: add instruction to install graphviz before requirements.txt test: add unit tests and refactor filter computer notebooks Nov 20, 2023
@danieljhegeman danieljhegeman marked this pull request as ready for review November 20, 2023 17:32
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

This PR has not seen any activity in the past 2 weeks; if no one comments or reviews it in the next 3 days, this PR will be closed.

@github-actions github-actions bot added the Stale label Dec 5, 2023
nayib-jose-gloria
nayib-jose-gloria reviewed Dec 5, 2023
View reviewed changes
scripts/compute_mappings/README.md
To update scripts, edit the notebooks and re-run the cli command to re-generate the python scripts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update: after discussion with Pablo / Clever Canary, I don't think there's a desire to keep the notebooks. So we can delete them and remove this guidance from the README

@github-actions github-actions bot removed the Stale label Dec 6, 2023
@github-actions GitHub Actions
Copy link
Contributor

This PR has not seen any activity in the past 2 weeks; if no one comments or reviews it in the next 3 days, this PR will be closed.

@github-actions github-actions bot added the Stale label Dec 21, 2023
@github-actions GitHub Actions
Copy link
Contributor

This PR was closed because it has been inactive for 17 days, 3 days since being marked as stale. Please re-open if you still need this to be addressed.

@github-actions github-actions bot closed this Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nayib-jose-gloria nayib-jose-gloria nayib-jose-gloria left review comments

Assignees
No one assigned
Labels
autoclosed Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danieljhegeman @nayib-jose-gloria