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

Update QC report to 0.13.0 #124

Merged
merged 7 commits into from
Apr 10, 2024
Merged

Update QC report to 0.13.0 #124

merged 7 commits into from
Apr 10, 2024

Conversation

fbdtemme
Copy link
Contributor

@fbdtemme fbdtemme commented Apr 9, 2024

Description

  • Remove cell calling plot
  • Subsample non cell components to improve ranked cell size plot performance

Fixes: EXE-1623, EXE-1625

@fbdtemme fbdtemme marked this pull request as draft April 9, 2024 15:19
@fbdtemme fbdtemme force-pushed the feature/update-qc-report-0.13.0 branch from d289e0b to b3637ef Compare April 9, 2024 16:04
@fbdtemme fbdtemme requested a review from johandahlberg April 9, 2024 16:05
@fbdtemme fbdtemme marked this pull request as ready for review April 9, 2024 16:05
@codecov Codecov
Copy link

codecov bot commented Apr 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.48%. Comparing base (d2a334a) to head (daa2165).
Report is 10 commits behind head on dev.

❗ Current head daa2165 differs from pull request most recent head a84fc42. Consider uploading reports for the commit a84fc42 to get more accurate results

Files Patch % Lines
src/pixelator/utils/simplification.py 94.59% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##              dev     #124      +/-   ##
==========================================
+ Coverage   81.37%   81.48%   +0.11%     
==========================================
  Files         114      115       +1     
  Lines        6302     6353      +51     
==========================================
+ Hits         5128     5177      +49     
- Misses       1174     1176       +2

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

johandahlberg
johandahlberg requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@johandahlberg johandahlberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should have a discussion about the simplification package. But we can take that offline. Other than that everything looks good to me.

Taskfile.yml
@@ -54,13 +54,13 @@ tasks:
desc: |-
Run tests using pytest with the default flags defined in pyproject.toml.
cmds:
- pytest tests
- pytest tests {{ .CLI_ARGS }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know you could to this. But it seems super useful.

pyproject.toml Outdated
@@ -54,6 +54,7 @@ fsspec = "^2023.12.2"
fastparquet = "^2023.8.0"
graspologic = "^3.3.0"
plotly = "*"
simplification = "^0.7.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see this package in conda-forge/bioconda - so I think we should think twice before brining it in.

@fbdtemme
Copy link
Contributor Author

I have removed the dependency and brought in a numpy based version adapted from https://github.com/fhirschmann/rdp
with a couple of fixes suggested in the issues there:

@fbdtemme fbdtemme requested a review from johandahlberg April 10, 2024 14:07
@fbdtemme
Copy link
Contributor Author

I have also tested this on some production runs and it is definitely fast enough for our use case right now.

johandahlberg
johandahlberg approved these changes Apr 10, 2024
View reviewed changes
src/pixelator/utils/simplification.py Outdated Show resolved Hide resolved
src/pixelator/utils/simplification.py Outdated
@@ -0,0 +1,119 @@
"""Implementation of the Ramer–Douglas–Peucker line simplification algorithm.

Based on https://github.com/fhirschmann/rdp.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to bring in the complete original copyright statement here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@fbdtemme @johandahlberg
Update src/pixelator/utils/simplification.py copyright statement
a84fc42 
Co-authored-by: Johan Dahlberg <johan.dahlberg@pixelgen.tech>
@fbdtemme fbdtemme merged commit d92067e into dev Apr 10, 2024
13 checks passed
@fbdtemme fbdtemme deleted the feature/update-qc-report-0.13.0 branch April 10, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johandahlberg johandahlberg johandahlberg approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@fbdtemme @johandahlberg