-
Notifications
You must be signed in to change notification settings - Fork 25
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
[python] test_registration_mappings.py::test_pandas_indexing
reformat
#2853
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2853 +/- ##
==========================================
+ Coverage 90.04% 90.19% +0.15%
==========================================
Files 37 37
Lines 3957 3957
==========================================
+ Hits 3563 3569 +6
+ Misses 394 388 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
"expected_signature": {"alt_id": "string", "obs_id": "string"}, | ||
}, | ||
], | ||
[ "index_col_and_name" , "default_index_name" , "signature_col_names" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this
I've also really struggled with auto-format of all the parametrize
blocks over the last couple years ... I don't know why I never used # fmt: off
/ #fmt: on
... regardless I'm delighted you're doing so
Moving forward I'll try to use this in some more places, some of the more complex parametrize
blocks ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I won't comment further since this is a draft PR -- just yay :) )
test_registration_mappings.py::test_pandas_indexing
reformattest_registration_mappings.py::test_pandas_indexing
reformat
They're both fine BTW (since you asked) :) |
The "(macos-12, 3.11) / lint" failure is the same as we were seeing yesterday, in #2839. One thing I see, that doesn't make sense, is this job's That is the same as was restored here, in a build which failed yesterday, which had different setup-python should just be sha256'ing |
Great catch @ryan-williams ! Please create a GitHub issue to track this. (I'll do the usual Shortcut replications.) |
Also, do we have a measurement of how much time the caching is saving? Perhaps we can live without it? I didn't see any quantification on #756.) Please let me know ... |
... oop, found this: #693 (comment) -- suggesting #756 saves us 40 seconds per run which IMO isn't worth the trouble -- the hours we've spent debugging caching issue this week far exceed even a high multiple of 40 seconds ... |
OK cool thanks @ryan-williams .... if it's possible to get some measurement on what the time savings really is, that'll make it easier to have an informed conversation on whether that particular caching is worth our effort .... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late approve! :|
Issue and/or context:
test_registration_mappings.py::test_pandas_indexing
is one of the tests closest to the issues described in #2829, but I struggled to parse what the 14 cases were verifying, so I reformatted and documented them in a way that I feel makes them easier to understand.Changes:
The first commit surfaces that the 14 cases are 2 similar blocks of 7 cases each (one where the
default_index_name
isobs_id
, one where it'salt_id
). Each set of 7 can be further subdivided into:default_index_name
matches the name of the column that was promoted todf.index
(anddf.index.name
is either preserved, set to"index"
, or set toNone
)df.index.name
is either preserved, set to"index"
, or set toNone
)df.index
is a defaultRangeIndex
, and both columns remain present)The 2nd commit inverts the grouping above (2x(3+3+1) → 2x3 + 2x3 + 2x1), and adds a comment above each group. Let me know if one presentation seems clearer than the other.
No functionality should have been changed, these are cosmetic changes.