fix PX timezone treatment #2749
Conversation
|
Ok this all works on my machine but fails all over CI. Looks like my approach is too brittle :) |
nicolaskruchten
force-pushed
the
px_tz_fix
branch
from
c9ba8aa
to
3601322
Compare
|
New approach succeeded: do everything in pandas-land instead of transiting through numpy-land at all :) Ready for review! |
|
|
||
| [testenv:py36-core] | ||
| basepython={env:PLOTLY_TOX_PYTHON_36:} | ||
| commands= | ||
| python --version | ||
| pytest {posargs} -x plotly/tests/test_core |
There was a problem hiding this comment.
why did you remove the -x ? Exiting on first failing test is good for the environment :-).
There was a problem hiding this comment.
I needed to be able to see if I fixed a test even if an "earlier" one failed. Looking at the timings, it seems to me that the container setup etc takes more time than the test suites proper, so while we have the container spun up we may as well extract as much information as possible about the run, is my feeling :)
| seems to mangle datetime columns. Stripping the index from existing pd.Series is | ||
| required to get things to match up right in the new DataFrame we're building | ||
| """ | ||
| return pd.Series(x).reset_index(drop=True) |
There was a problem hiding this comment.
this makes a copy of the series because of the reset_index step, so it's suboptimal for non datetime columns. Could a solution be to return x.values in other cases?
There was a problem hiding this comment.
Hmmm yeah we weren't copying before but we are now. After spending like 8 hours trying various permutations here I think I'm OK with the copy on the principle of "make it correct, then make it fast". We can optimize it later if this really causes problems.
Closes #2311 essentially by removing calls to
pd.Series.values, which seems to always convert datetimes to UTC.modified existing tests.