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

(issue 900): implemented Range.add_plot() and add_table() #926

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alixdamman
Copy link
Collaborator

TODO @alixdamman : add method documentation + update Tutorial

@gdementen
Copy link
Contributor

I wonder if it wouldn't be more logical to invert position and range. In test_make_plot that would be:

sheet["B8"].make_plot("B2")

Also, the dump and make_plot calls look alien to each other. One is a method on the source, the other on the target.

sheet["B2"] = population_be.dump()
sheet["B2"].make_plot("B8")

It would be better to have similar syntaxes for both. Either:

sheet["B2"] = population_be.dump()
sheet["B8"] = population_be.plot(data="B2")  # <-- bad syntax because population_be is not really used UNLESS we make it dump the data too

or

sheet["B2"].add_table(population_be)
sheet["B8"].add_plot(data_source="B2")

@gdementen
Copy link
Contributor

implementing add_table and add_plot could be a way to tackle #826, #900, and #967.

@alixdamman alixdamman requested a review from gdementen January 26, 2022 14:08
larray/inout/xw_excel.py Outdated Show resolved Hide resolved
larray/inout/xw_excel.py Outdated Show resolved Hide resolved
@gdementen
Copy link
Contributor

  • Shouldn't this be documented in the tutorial?
  • Shouldn't add_table create Excel tables (potentially as an option)? I remember adding some code which could be useful for that in the mcafee manager.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alixdamman
Copy link
Collaborator Author

Shouldn't add_table create Excel tables (potentially as an option)?

What do you mean by "Excel tables" ?

@gdementen
Copy link
Contributor

Shouldn't add_table create Excel tables (potentially as an option)?

What do you mean by "Excel tables" ?

A data range which uses the "format as Table" Excel feature (usually with an header row and autofilters).

See: https://support.microsoft.com/en-us/office/format-an-excel-table-6789619f-c889-495c-99c2-2f971c0e2370

@gdementen gdementen changed the title (issue 900): implemented Range.make_plot() (issue 900): implemented Range.add_plot() and add_table() Jul 6, 2022
@gdementen
Copy link
Contributor

to be closer to a replacement for the ExcelReport API, we could allow passing an array as the datasource and add a data_sheet (or dump_data_to or whatever it is called) argument to add_plot;

sheet["B8"].add_plot(data_source=population_be, data_sheet="data")

would be equivalent to:

datasheet = workbook[data_sheet]
pos = (datasheet.shape[0], 0)
datasheet[pos] = population_be.dump()
sheet["B8"].add_plot(data_source=datasheet[pos])

@gdementen gdementen added this to the 0.34 milestone Sep 16, 2022
@gdementen
Copy link
Contributor

Tentatively added to 0.34 milestone (using the assumption that I take over your code), unsure I will have time to do that.

@alixdamman
Copy link
Collaborator Author

Tentatively added to 0.34 milestone (using the assumption that I take over your code), unsure I will have time to do that.

OK. Please do.

@gdementen gdementen self-assigned this Sep 20, 2022
@gdementen gdementen modified the milestones: 0.34, 0.35 Feb 9, 2023
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.

2 participants