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

Fix display of non-numeric DataFrame index columns #5254

Merged
merged 5 commits into from
Mar 24, 2021

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Mar 23, 2021

For #5253

The change here is to show the index column in addition to the row number column only for pandas DataFrames and Series objects, because index columns are meaningful for DFs and Series but for ndarrays, lists, dicts and tensors the index column is generated by us as part of the DataFrame conversion code and is hence not meaningful to the user.

DataFrame:
image

Same data as an ndarray (note that there isn't actually an index column being generated on the ndarray itself):
image

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

@codecov-io
Copy link

codecov-io commented Mar 23, 2021

Codecov Report

Merging #5254 (79b002b) into main (672c145) will increase coverage by 0%.
The diff coverage is n/a.

❗ Current head 79b002b differs from pull request most recent head 7162579. Consider uploading reports for the commit 7162579 to get more accurate results

@@          Coverage Diff          @@
##            main   #5254   +/-   ##
=====================================
  Coverage     73%     73%           
=====================================
  Files        401     401           
  Lines      26522   26532   +10     
  Branches    3869    3872    +3     
=====================================
+ Hits       19441   19453   +12     
+ Misses      5476    5474    -2     
  Partials    1605    1605           
Impacted Files Coverage Δ
...datascience/notebook/defaultCellLanguageService.ts 91% <0%> (-6%) ⬇️
...ent/datascience/notebook/notebookEditorProvider.ts 87% <0%> (-2%) ⬇️
src/client/datascience/dataScienceSurveyBanner.ts 81% <0%> (-1%) ⬇️
src/client/datascience/notebook/remoteSwitcher.ts 91% <0%> (+<1%) ⬆️
src/client/datascience/baseJupyterSession.ts 76% <0%> (+<1%) ⬆️
src/client/datascience/raw-kernel/rawSocket.ts 86% <0%> (+1%) ⬆️
...rc/client/datascience/jupyter/debuggerVariables.ts 78% <0%> (+3%) ⬆️

@joyceerhl joyceerhl marked this pull request as ready for review March 24, 2021 15:16
@joyceerhl joyceerhl requested a review from a team as a code owner March 24, 2021 15:16
@@ -170,6 +170,17 @@ def _VSCODE_getDataFrameInfo(df):

columnTypes = _VSCODE_builtins.list(df.dtypes)

# Compute the index column. It may have been renamed
try:
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit different from the code in getJupyterVariableDataFrameInfo.py that does something similar with index column. Looks like your version might be safer with the try / except. Should that maybe be updated as well?

Copy link
Contributor Author

@joyceerhl joyceerhl Mar 24, 2021

Choose a reason for hiding this comment

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

This is actually just restoring code that I removed several PRs ago. Rich pointed out at the time that he thought it was required, and now I see why 😊

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, maybe I didn't say that right. I was saying that this code here looked safer than the code in the other location. Just wondering if getJupyterVariableDataFrameInfo.py should be upgraded with the try / except that you have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, will update

@joyceerhl joyceerhl merged commit 4c4a36a into main Mar 24, 2021
@joyceerhl joyceerhl deleted the joyceerhl/df-index-fix branch March 24, 2021 18:12
DonJayamanne added a commit to jakebailey/vscode-jupyter that referenced this pull request Mar 25, 2021
* main: (33 commits)
  Fix problems found while debugging synapse connections (microsoft#5297)
  Ask user to enable CDN support to render IPyWidget (microsoft#5282)
  Remove jupyter.useNotebookEditor (microsoft#5249)
  Fixes to creating and re-opning non-python notebooks (microsoft#5271)
  Update main part 2 (microsoft#5296)
  Fix problem with run selection in an editor (microsoft#5298)
  Avoid calling `reveal` if a webview is already active or visible (microsoft#5276)
  Remove default value from setting description (microsoft#5288)
  Use 'learn more' clickable link for setting (microsoft#5285)
  update to next version number (microsoft#5275)
  Fix display of non-numeric DataFrame index columns (microsoft#5254)
  Use resolveKernel instead of resolveDocument which is due for deprecation and re-enable and update associated tests. (microsoft#5255)
  Display survey only when dealing with Jupyter Notebooks (microsoft#5248)
  Remove java kernel tests from Notebooks (microsoft#5247)
  Fix scheduled build pipeline (microsoft#5245)
  Add string argument to jupyter.execSelectionInteractive for extensibility (microsoft#5053)
  Finally to make sure that we claen up our status message (microsoft#5239)
  Updates to Notebook kernel execution API (microsoft#5241)
  Changes to execution of Native Notebooks (changes in Proposed API) (microsoft#5180)
  Update release pipeline (microsoft#5231)
  ...
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.

3 participants