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

Fix marker ordering errors and verification for channel enrichment #274

Merged
merged 7 commits into from
Oct 16, 2020
Merged

Fix marker ordering errors and verification for channel enrichment #274

merged 7 commits into from
Oct 16, 2020

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Addresses and closes #265. Also addresses and closes #266. We add a more robust method to verify that the markers in all_channel_data and marker_thresholds are the same between the two, and we ensure that they are ordered consistently as well. This only applies for channel enrichment.

How did you implement your changes

Use set equality to check if the markers are the same, and reindexing to enforce order.

Remaining issues

This is an extremely strict way of handling the various different types of marker_thresholds and all_channel_data values the user may pass in. From personal experience, expect all sorts of messes in this regard. This method may be a bit too strict, so I'm open to other ideas.

@alex-l-kong alex-l-kong self-assigned this Oct 15, 2020
@alex-l-kong alex-l-kong changed the title Fix marker ordering errors and verification Fix marker ordering errors and verification for channel enrichment Oct 15, 2020
ngreenwald
ngreenwald requested changes Oct 15, 2020
View reviewed changes
ark/utils/test_utils.py Outdated Show resolved Hide resolved
ngreenwald
ngreenwald requested changes Oct 15, 2020
View reviewed changes
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Ah right, got confused by the comment, I see now. Just a one-liner check for new test and we should be good

ark/analysis/spatial_analysis.py Show resolved Hide resolved
ackagel
ackagel approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@ackagel ackagel left a comment

Choose a reason for hiding this comment

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

marker_thresholds is growing up so fast 😭 ; so much tougher and robust than they once were

@alex-l-kong alex-l-kong merged commit c2caf48 into master Oct 16, 2020
@alex-l-kong alex-l-kong deleted the marker_verify branch October 16, 2020 19:56
alex-l-kong added a commit that referenced this pull request Jan 14, 2021
@alex-l-kong
Fix marker ordering errors and verification for channel enrichment (#274
88b013a 
)

* Proposed method of new marker ordering verification

* Remove TODO comment (addressed in PR)

* Remove extra print statement

* Make test for marker verification more complete
y2kbugger pushed a commit that referenced this pull request Jul 29, 2021
@alex-l-kong
Fix marker ordering errors and verification for channel enrichment (#274
1b3dab0 
)

* Proposed method of new marker ordering verification

* Remove TODO comment (addressed in PR)

* Remove extra print statement

* Make test for marker verification more complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ackagel ackagel ackagel approved these changes

@ngreenwald ngreenwald ngreenwald approved these changes

Assignees

@alex-l-kong alex-l-kong

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@alex-l-kong @ngreenwald @ackagel