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

Consolidate hypothesis testing 178390751 #320

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ernestoarbitrio
Copy link
Contributor

@ernestoarbitrio ernestoarbitrio commented Jun 7, 2021

Notion page: https://www.notion.so/crunch/Sort-by-value-Transform-for-Dashboards-and-Export-PRD-759062891aa2471e8f9ca9c766ee6e72#440ab7f0a4234e8eb811678762ffb17a

Goal: support sort-by-value for pairwise significance measures.

All pairwise significance measures have been moved from methods to lazyproperties.

The new way to specify the pairwise sig column baseline is to use a new object in the transforms:

        transforms = {
            "columns_dimension": {
                "pairwise_significance": {
                    "column_significance": {"element_id": 0}  # < - normal column
                }
            }
        }

        transforms = {
            "columns_dimension": {
                "pairwise_significance": {
                    "column_significance": {
                        "insertion_id": 1 or "str_id"  # < - insertion column
                    }
                }
            }
        }

@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch 3 times, most recently from 1ee1227 to 6ef195c Compare June 8, 2021 09:32
@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #320 (ea06064) into master (80986d5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #320    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           20        19     -1     
  Lines         4639      4848   +209     
==========================================
+ Hits          4639      4848   +209     
Impacted Files Coverage Δ
src/cr/cube/cube.py 100.00% <100.00%> (ø)
src/cr/cube/cubepart.py 100.00% <100.00%> (ø)
src/cr/cube/dimension.py 100.00% <100.00%> (ø)
src/cr/cube/matrix/assembler.py 100.00% <100.00%> (ø)
src/cr/cube/matrix/measure.py 100.00% <100.00%> (ø)

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 80986d5...ea06064. Read the comment docs.

@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch 8 times, most recently from bfc1057 to f9ab2ba Compare June 10, 2021 10:17
@ernestoarbitrio ernestoarbitrio marked this pull request as ready for review June 10, 2021 10:21
@ernestoarbitrio ernestoarbitrio requested a review from scanny June 11, 2021 08:58
Copy link
Contributor

@scanny scanny left a comment

Choose a reason for hiding this comment

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

Okay, I'm out of time for today, but here are some to get you started.

src/cr/cube/cube.py Outdated Show resolved Hide resolved
src/cr/cube/cube.py Outdated Show resolved Hide resolved
src/cr/cube/cubepart.py Outdated Show resolved Hide resolved
src/cr/cube/cubepart.py Show resolved Hide resolved
src/cr/cube/cubepart.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch 6 times, most recently from 4f99665 to 161e9e9 Compare June 17, 2021 17:50
@ernestoarbitrio ernestoarbitrio requested a review from scanny June 17, 2021 17:51
Copy link
Contributor

@scanny scanny left a comment

Choose a reason for hiding this comment

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

Okay, I didn't get far when I discovered a basic error in how this is organized. Indices need to be computed in the measure and not in assembler. Then assembler's role is just to adjust the indices from payload-order to sorted-order. Everything else (like ._assemble_matrix happens the same way as any other measure.

src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
src/cr/cube/matrix/assembler.py Outdated Show resolved Hide resolved
@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch 7 times, most recently from 0a1021a to 2f55a82 Compare June 30, 2021 11:02
@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch 2 times, most recently from 56b1896 to 92db5fd Compare June 30, 2021 15:30
@ernestoarbitrio ernestoarbitrio requested a review from scanny June 30, 2021 15:31
Copy link
Contributor

@scanny scanny left a comment

Choose a reason for hiding this comment

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

Okay, LGTM after some docstring improvement :)

src/cr/cube/cubepart.py Outdated Show resolved Hide resolved
@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch from 92db5fd to 8b73013 Compare July 2, 2021 06:28
@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch from 41c3f50 to 8f63631 Compare September 2, 2021 10:02
@ernestoarbitrio ernestoarbitrio requested review from scanny, gergness and slobodan-ilic and removed request for slobodan-ilic September 2, 2021 15:50
Copy link
Contributor

@gergness gergness left a comment

Choose a reason for hiding this comment

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

I had originally wanted to go through and understand the whole thing, which I haven't had time to do yet. (Not sure what you were asking for, but since Steve already approved, is it good?)

One comment about a docstring change

@@ -1300,7 +1300,7 @@ def reference_column_idx(self):
"""Optional int index of base column as reference for the pairwise sig tests."""
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring is out of date now, right?
"Optional int index" -> "Optional element id" (which is either an int/str if you want to have the type)

(I think Mike's arguing in slack that this should be called the "code", but this would be the only place we use that terminology)

@ernestoarbitrio ernestoarbitrio force-pushed the consolidate-hypothesis-testing-178390751 branch from f82bb2d to ea06064 Compare October 8, 2021 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants