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

[KED-2906] Add new plotly.JSONDataSet #981

Merged
merged 23 commits into from
Oct 26, 2021
Merged

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Oct 22, 2021

Description

Following #839 and #955, we now have a new dataset plotly.JSONDataSet.
image

The old plotly.PlotlyDataSet still works exactly as it did before:
image

The inheritance structure has been rewritten and the code tided up. plotly.JSONDataSet should be considered the "base" class that does the heavy lifting of converting between plotly figure and json; plotly.PlotlyDataSet subclasses this and is a convenience wrapper that enables you to make plots directly from a pandas DataFrame.

Tested manually with both local and remote (s3) storage.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change and added my name to the list of supporting contributions in the RELEASE.md file
  • Added tests to cover my changes

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Comment on lines +149 to +164
def _load(self) -> Union[go.Figure, go.FigureWidget]:
load_path = get_filepath_str(self._get_load_path(), self._protocol)

with self._fs.open(load_path, **self._fs_open_args_load) as fs_file:
# read_json doesn't work correctly with file handler, so we have to read
# the file, decode it manually and pass to the low-level from_json instead.
return pio.from_json(str(fs_file.read(), "utf-8"), **self._load_args)

def _save(self, data: go.Figure) -> None:
save_path = get_filepath_str(self._get_save_path(), self._protocol)

with self._fs.open(save_path, **self._fs_open_args_save) as fs_file:
data.write_json(fs_file, **self._save_args)

self._invalidate_cache()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is essentially the same as what plotly.PlotlyDataSet did but without conversion from a pandas dataframe first.

from kedro.io.core import Version, get_filepath_str
from kedro.io.core import Version

from .json_dataset import JSONDataSet


class PlotlyDataSet(JSONDataSet):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this now inherits from the new plotly.JSONDataSet class, not pandas.JSONDataSet like it used to.

@antonymilne antonymilne changed the title Add new plotly.JSONDataSet [KED-2906] Add new plotly.JSONDataSet Oct 22, 2021
@antonymilne antonymilne marked this pull request as ready for review October 22, 2021 15:03
Copy link
Contributor

@limdauto limdauto left a comment

Choose a reason for hiding this comment

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

LGTM!

version=self._version,
)

def _load(self) -> Union[go.Figure, go.FigureWidget]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Union intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You can specify the output_type (through load_args): https://plotly.com/python-api-reference/generated/plotly.io.from_json.html

setup.py Outdated
@@ -106,7 +106,10 @@ def _collect_requirements(requires):
"pandas.SQLQueryDataSet": [PANDAS, "SQLAlchemy~=1.2"],
}
pillow_require = {"pillow.ImageDataSet": ["Pillow~=8.0"]}
plotly_require = {"plotly.PlotlyDataSet": [PANDAS, "plotly~=4.14"]}
plotly_require = {
"plotly.PlotlyDataSet": [PANDAS, "plotly~=4.14"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we can relax this even further to ~=4.0 to be honest.

)
def plotly_args(request):
return request.param
@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
I hereby nominate myself for a values award.

Copy link
Member

Choose a reason for hiding this comment

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

🤣

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

setup.py Outdated Show resolved Hide resolved
@antonymilne antonymilne merged commit f333099 into master Oct 26, 2021
@antonymilne antonymilne deleted the feature/new-plotly-dataset branch October 26, 2021 09:28
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
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.

4 participants