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

Python Script Rewrite #219

Closed
wants to merge 48 commits into from
Closed

Conversation

irgolic
Copy link
Member

@irgolic irgolic commented Sep 25, 2020

Description of changes

Now featuring:

  • IPython console
  • Enhanced editor (vim mode)
  • Autocompleter
  • Library, stored in a folder
Includes
  • Code changes
  • Tests
  • Documentation

@borondics
Copy link
Member

This would be a great addition!

@irgolic
Copy link
Member Author

irgolic commented Sep 25, 2020

The reason this isn't in core is because the kernel runs in a separate process, so all signals need to be copied to/from it.

When we do biolab/orange-widget-base#86, this won't be a problem, and the widget will be viable for core.

@irgolic irgolic force-pushed the python-script-ipython branch from 6339459 to 1d69ec8 Compare September 29, 2020 14:18
…on qtconsole

- Introduce OrangeIPythonKernel for script running functionality
- Modelled after Jupyter lab
- Editor is styled with Qutepart's built-in highlighter
- Console is styled with pygments
The subclassed widget uses a lot of super protected methods, mostly in
manipulating prompt appearance -- any bugs which arise are likely to be purely
cosmetic. Still, I'm constraining the required version to the current latest version.

- Implement kernel interrupt on new run_script request or ctrl+C
by adding a fake function signature and return statement,
whose variables are bolded on in_ signal connection/out_ variable collection.

- Removed info box
- TODO add tooltips to parameters
@irgolic irgolic force-pushed the python-script-ipython branch from 1d69ec8 to 84d2019 Compare December 4, 2020 16:03
@ajdapretnar
Copy link
Contributor

I like this widget.

I would suggest a few improvements, though (most can be done in a separate pull request).

  • Use native data tables should, imo, be the default.
  • Autocomplete should, imo, not fill the first value when at dot. Example: I type in_data. and then I want to see the options. Tab should not work as select. That should be Enter. Tab should just list the possible values. In Console, tab navigates through values.
  • Library window could be smaller.

@irgolic
Copy link
Member Author

irgolic commented Dec 7, 2020

Autocomplete should, imo, not fill the first value when at dot. Example: I type in_data. and then I want to see the options. Tab should not work as select. That should be Enter. Tab should just list the possible values. In Console, tab navigates through values.

I'll look into this. I don't remember this exact detail, but I'll try to make it as similar to PyCharm/Jupyter as possible.

Library window could be smaller.

Should I make it resizeable?

Use native data tables should, imo, be the default.

I must disagree, most people are familiar with pandas, and look for ways to use pandas in Orange. One of the strongest features of the widget is easy pandas use.

I'll fix the documentation, then this should be ok to merge.

@ajdapretnar
Copy link
Contributor

Should I make it resizeable?

Yes, that'd be nice.

I must disagree, most people are familiar with pandas, and look for ways to use pandas in Orange. One of the strongest features of the widget is easy pandas use.

The input of any widget to Python Script is Orange.data.Table, not pandas DataFrame. So nothing will work on Orange tables unless someone knows how to manipulate these. Also, this would break any backwards compatibility.

@borondics
Copy link
Member

Maybe adding a button inside the widget to switch between the data types (Orange/Pandas) or even just providing two variables inside the ipython environment (Orange.Table and Pandas.DF) would solve this. The problem might be that it duplicates data. Or, another idea is to have some very simple to use converter functions available as part of the orange python module so that the user can decide. After all those who use Python Script should be able to script.

@irgolic
Copy link
Member Author

irgolic commented Dec 7, 2020

The input of any widget to Python Script is Orange.data.Table, not pandas DataFrame. So nothing will work on Orange tables unless someone knows how to manipulate these.

I'm sorry, I don't understand – the input is Orange.data.Table. If the 'Use native data tables' setting is false, the table is automatically transformed into pandas.DataFrame, and the output is automatically transformed from a pandas.DataFrame into an Orange.data.Table. This is shown to the user by the fake function signature and return statement (in_df instead of in_data, out_df instead of out_data).

I understand how this can be confusing to those with experience in Orange python scripting. I tried to make it readily obvious with the fake function signature and return statement, which change instantly upon flicking the setting's checkbox.

Also, this would break any backwards compatibility.

When moving to core, I'll add a migration that sets the setting to true.

Maybe adding a button inside the widget to switch between the data types (Orange/Pandas)

This is how it is currently, with a checkbox.

providing two variables inside the ipython environment (Orange.Table and Pandas.DF) would solve this

Could do, but I'm unsure how to nicely show this in the fake function signature/return statement without adding confusion.

Or, another idea is to have some very simple to use converter functions available as part of the orange python module so that the user can decide.

They exist, and they're used to automatically transform the Table to/from DataFrame, if the setting is false. Learning them adds an avoidable bump on Orange's learning curve.

@kernc
Copy link
Contributor

kernc commented Dec 7, 2020

I understand how this can be confusing to those with experience in Orange python scripting. I tried to make it readily obvious with the fake function signature and return statement, which change instantly upon flicking the setting's checkbox.

Why not add a few UserAdviceMessages for good measure.

This is how it is currently, with a checkbox.

The more correct would be a toggle button. Traditionally, buttons effect on click, checkboxes on form submission.

@markotoplak
Copy link
Member

markotoplak commented Dec 7, 2020

The input is Orange.data.Table. If the 'Use native data tables' setting is false, the table is automatically transformed into pandas.DataFrame, and the output is automatically transformed from a pandas.DataFrame into an Orange.data.Table. This is shown to the user by the fake function signature and return statement (in_df instead of in_data, out_df instead of out_data).

Converting into data frame (and back) requires an additional data copy. Also, the native data table in Orange is numpy. Could you rename it to "work with Pandas data frames (or whatever, just mention pandas...)" with a default False (due to copying).

Also, going through pandas and back will lose table descriptors, such as finer parts of a domain. So its outputs could have problems regarding further subsets and data processing. It would be fine if the user just wanted to use Python scripts as a dead end widget, to compute statistics/display graphs.

@markotoplak
Copy link
Member

But I'd prefer if the users had to do these conversions in code explicitly. Adding that line to the docs / sample scripts could suffice.

@borondics
Copy link
Member

(OK, at this point it is obvious that I am not checking either the code or the widget itself, just talking in general) How about keeping everything in the Orange.Table format except providing the .X part as a pandas DF? Then it could be put back to the Table.X for the output if the .shape parameter didn't change.

As @markotoplak says the domain will have problems here (e.g. Spectroscopy keeps the x-axis values in features).

@janezd
Copy link
Contributor

janezd commented Dec 8, 2023

Some of this has already been moved to the core widget, the rest is in a PR there.

@janezd janezd closed this Dec 8, 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.

6 participants