Handle timestamp and nans in removing multi index failure cases #1516
Conversation
rorymcstay
force-pushed
the
bugfix/handle-timestamp-index-on-dropping-multiindex
branch
from
27fb29c
to
a770bda
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1516 +/- ##
==========================================
- Coverage 94.28% 93.30% -0.98%
==========================================
Files 91 120 +29
Lines 7013 9133 +2120
==========================================
+ Hits 6612 8522 +1910
- Misses 401 611 +210 ☔ View full report in Codecov by Sentry. |
|
@rorymcstay thanks! need to run pre-commit to fix linter errors: https://pandera.readthedocs.io/en/stable/CONTRIBUTING.html#set-up-pre-commit |
rorymcstay
force-pushed
the
bugfix/handle-timestamp-index-on-dropping-multiindex
branch
2 times, most recently
from
1a4c817
to
2a18b64
Compare
…ai-oss#1469 Signed-off-by: Rory <rory@rorymcstay.com> Signed-off-by: Rory McStay <rory.mcstay@fulcrumasset.com>
rorymcstay
force-pushed
the
bugfix/handle-timestamp-index-on-dropping-multiindex
branch
from
2a18b64
to
ef5515f
Compare
@cosmicBboy thanks, finally came back round to this and updated the PR |
| err.failure_cases["index"] | ||
| .astype(str) | ||
| .apply( | ||
| lambda i: eval(i, _MULTIINDEX_HANDLED_TYPES) # pylint: disable=eval-used |
There was a problem hiding this comment.
hmm, I didn't realize when this eval snuck into the codebase... are you aware of a more secure way of doing this @rorymcstay ?
There was a problem hiding this comment.
looks like I missed the eval change reviewing this change: d43a11b
We don't need to address in this PR, but if you can think of a non-eval approach to this in a separate PR that would be awesome!
There was a problem hiding this comment.
Approving and merging this now @rorymcstay.
If you have any thoughts on https://github.com/unionai-oss/pandera/pull/1516/files#r1718793226 I'd appreciate any ideas you may have!
|
Thanks, I was trying to recall where the index is cast to a string to consider changing its implementation. When I investigated this issue initially, I wasn't sure on how to proceed, as I considered it a breaking change to no longer cast to string. Once I get back round to finding where this was done, I will see what the changes would look like. |
|
@cosmicBboy Do you think we could merege this now? |
I see some failed merge checks, please ignore whilst i investigate |
|
@cosmicBboy what should the approach here be regarding project code coverage. I would need to add tests which aren't relevant to this PR IIUC. |
Fixes the following issue
#1469
Longer term, there probably needs to be another way to track invalid records without serialising the index to string..