Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix #17 - cuIndicator notebook plot widget is too complicated (WIP) #45

Merged
merged 2 commits into from
Aug 16, 2019
Merged

Fix #17 - cuIndicator notebook plot widget is too complicated (WIP) #45

merged 2 commits into from
Aug 16, 2019

Conversation

miguelusque
Copy link
Contributor

Hi,

This is a WIP of issue #17.

It is not finished, but it is much better than the existing example in master branch.

It is also needed because it makes use of TaskGraph and TaskSpecSchema classes.

Therefore, I would like it to be merged into master this week, and I will continue polishing it in the next few days.

Regards,
Miguel

P.S.: Final refactoring is subject to change significantly. I am still designing the final solution.

@avolkov1
Copy link
Contributor

The create_figure could be refactored. It looks the same. Only the label or title seems to differ.

# module: viz_common.py

def create_figure(title, stock, dt_scale, sc, color_id, f, indicator_figure_height, figure_width, add_new_indicator):
    sc_co = LinearScale()
    ax_y = Axis(label=title, scale=sc_co, orientation='vertical')
    new_line = Lines(x=stock.datetime, y=stock['out'], scales={'x': dt_scale, 'y': sc_co}, colors=[CATEGORY20[color_id[0]]])
    new_fig = Figure(marks=[new_line], axes=[ax_y])
    new_fig.layout.height = indicator_figure_height
    new_fig.layout.width = figure_width                    
    figs = [new_line]
    # add new figure
    add_new_indicator(new_fig)
    return figs

Other then create_figure the other functions differ so it's hard to unify them. But cell 5 is still huge. Pretty much that whole cell 5 should be in a module and imported. Refer to plotutils.py and 06_xgboost_trade.ipynb. It should be like that ideally.

@miguelusque
Copy link
Contributor Author

Hi Alex,

I will follow your comments in the next PR.

Cell #5 is still huge, but much smaller than before.

If you don’t mind, I’d like to merge this version. I am aware there is a lot of room for improvement in this notebook, but it is better than the current version in master.

I’ll try to have a definitive version of this notebook by the end of next week.

Regards,
Miguel

@avolkov1
Copy link
Contributor

Hi Alex,

I will follow your comments in the next PR.

Cell #5 is still huge, but much smaller than before.

If you don’t mind, I’d like to merge this version. I am aware there is a lot of room for improvement in this notebook, but it is better than the current version in master.

I’ll try to have a definitive version of this notebook by the end of next week.

Regards,
Miguel

Fine by me.

@yidong72
Copy link
Collaborator

Fine with me too

@avolkov1
Copy link
Contributor

Alright, which one of us is gonna merge this.

@yidong72
Copy link
Collaborator

@avolkov1, can you do it? since you do most of the review for this.

@avolkov1
Copy link
Contributor

@avolkov1, can you do it? since you do most of the review for this.

No problem. I'll merge this now then.

@avolkov1 avolkov1 merged commit 7dd1817 into NVIDIA:develop Aug 16, 2019
@miguelusque miguelusque deleted the fix_17 branch August 16, 2019 16:48
@avolkov1 avolkov1 mentioned this pull request Aug 16, 2019
@miguelusque miguelusque changed the title Fix 17 - cuIndicator notebook plot widget is too complicated (WIP) Fix #17 - cuIndicator notebook plot widget is too complicated (WIP) Aug 20, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants