Skip to content

Fix usage of column_names which is part of Interchange protocol#4442

Merged
alexcjohnson merged 3 commits intoplotly:masterfrom
anmyachev:fix-column-names
Nov 28, 2023
Merged

Fix usage of column_names which is part of Interchange protocol#4442
alexcjohnson merged 3 commits intoplotly:masterfrom
anmyachev:fix-column-names

Conversation

@anmyachev
Copy link
Copy Markdown
Contributor

@anmyachev anmyachev commented Nov 27, 2023 •
edited
Loading

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

@anmyachev
Fix usage of 'column_names' that part of Interchange protocol
bc8f920 
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev
Copy link
Copy Markdown
Contributor Author

anmyachev commented Nov 27, 2023

Hi @MarcoGorelli! Could you take a look?

@anmyachev anmyachev mentioned this pull request Nov 27, 2023
Closed
3 tasks
@MarcoGorelli
Copy link
Copy Markdown
Contributor

MarcoGorelli commented Nov 28, 2023

looks fine

@anmyachev anmyachev marked this pull request as ready for review November 28, 2023 16:32
@anmyachev
Copy link
Copy Markdown
Contributor Author

anmyachev commented Nov 28, 2023

looks fine

@MarcoGorelli thanks for the feedback!

@anmyachev
Copy link
Copy Markdown
Contributor Author

anmyachev commented Nov 28, 2023

Hi @alexcjohnson! I'm not sure if it's necessary to add tests here, do you think it's worth adding? The change looks safe enough.

For example, I could add a synthetic test where the generator from this function will be returned instead of list.

plotly.py/packages/python/plotly/plotly/tests/test_optional/test_px/test_px_input.py

Line 261 in 63b9ac5

return self._columns

@alexcjohnson
Copy link
Copy Markdown
Collaborator

alexcjohnson commented Nov 28, 2023

I see, so in principle column_names could be a non-materialized list - generator, iterator, whatever, but we actually depend on columns being materialized, indexable etc... and so far all the dataframes we've encountered have it materialized but in principle that could change any time and still be compliant with the protocol. Do I have that right?

If so, then yes, I think your proposed test would be great. Given that this PR is fixing a case we hadn't previously covered it definitely wants a test one way or another, that would have failed without this change. After that the only other thing we'll want is a changelog entry.

Thanks for the PR!

@anmyachev
Copy link
Copy Markdown
Contributor Author

anmyachev commented Nov 28, 2023

I see, so in principle column_names could be a non-materialized list - generator, iterator, whatever, but we actually depend on columns being materialized, indexable etc... and so far all the dataframes we've encountered have it materialized but in principle that could change any time and still be compliant with the protocol. Do I have that right?

Yes, you are right. Thanks for the quick response!

If so, then yes, I think your proposed test would be great. Given that this PR is fixing a case we hadn't previously covered it definitely wants a test one way or another, that would have failed without this change. After that the only other thing we'll want is a changelog entry.

I added a test and a log entry.

@anmyachev
add changelog entry and test case
4403972 
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev
fix
e7bfc7c 
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
anmyachev
anmyachev commented Nov 28, 2023
View reviewed changes
packages/python/plotly/plotly/tests/test_optional/test_px/test_px_input.py
@pytest.fixture
def add_interchange_module_for_old_pandas():
if not hasattr(pd.api, "interchange"):
pd.api.interchange = mock.MagicMock()
Copy link
Copy Markdown
Contributor Author

@anmyachev anmyachev Nov 28, 2023

Choose a reason for hiding this comment

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

This change is necessary so that this attribute is not saved between runs.

alexcjohnson
alexcjohnson approved these changes Nov 28, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Very nicely done @anmyachev - thanks again!

@alexcjohnson alexcjohnson merged commit 8bc43e5 into plotly:master Nov 28, 2023
@anmyachev anmyachev deleted the fix-column-names branch November 28, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anmyachev @MarcoGorelli @alexcjohnson