Skip to content

DM-9943 Create Plotly version of histogram #343

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 9, 2017
Merged

DM-9943 Create Plotly version of histogram #343

merged 3 commits into from
Apr 9, 2017

Conversation

cwang2016
Copy link
Contributor

@cwang2016 cwang2016 commented Apr 5, 2017 •
edited
Loading

This development made a histogram conversion from hi-chart to Plotly based, including

  • construct Plotly accepted data and layout JSON structure based on the parameters passed to the 'Histogram' component.
  • Try to make the Plotly based histogram with similar styling as current hi-chart based histogram.

note: scatter chart on phase folding in light curve (LcPeriodPlotly.jsx) is trimmed and cleaned a little bit.

test:
start a catalog search
click 'charts' from the menu in the tri-view page to select 'histogram' and the column to make histogram chart

@cwang2016 cwang2016 self-assigned this Apr 5, 2017
tgoldina
tgoldina approved these changes Apr 6, 2017
View reviewed changes
Copy link
Contributor

@tgoldina tgoldina left a comment

Choose a reason for hiding this comment

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

Looks very nice!

I like the bins without border, so that the chart blends in with other widgets when in tri-view, but this is just a personal preference.

src/firefly/js/charts/ui/HistogramPlotly.jsx Outdated
r: 30,
b: 60,
t: 30
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When axis side changes (bottom/top or left/right), the margin needs to change. Plotly does not handle it for us.

src/firefly/js/charts/chartTypes/HistogramTblView.jsx Outdated
@@ -38,7 +40,9 @@ function Chart(props) {
if (!TblUtil.isFullyLoaded(tblId) || !chartData || !heightPx || !widthPx) {
return (<div/>);
}
const { isDataReady, data:histogramData, options:histogramParams} = ChartsCntlr.getChartDataElement(chartId);
var { isDataReady, data:histogramData, options:histogramParams} = ChartsCntlr.getChartDataElement(chartId);
const HistogramInstance = get(getAppOptions(), 'charts.chartEngine') !== 'plotly' ? Histogram : HistogramPlotly;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest renaming Histogram to HistogramHighcharts and have Histogram.jsx handle instance creation, like you do here. This way all references to Histogram will be switched to plotly references without changes in the code that uses it. (We have a few references besides HistogramTblView.)

@tgoldina
Copy link
Contributor

tgoldina commented Apr 6, 2017

Another thing I noticed – we can address it in a separate bug ticket if it takes long time to fix – X log option produces Invalid data passed to Histogram: TypeError: Cannot read property '1' of undefined message and histogram is not displayed.

Test case:
catalog search m31, radius 200 arcsec,
create fixed bin histogram for w1mpro, then apply X log option.

loitly
loitly approved these changes Apr 6, 2017
View reviewed changes
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

It looks good. The only thing I noticed that's different is you have to click on the edges to select the chart(yellow highlight). If possible, it should work like it used to, clicking anywhere in the chart selects it.

src/firefly/js/charts/ui/HistogramPlotly.jsx Outdated

return (
<div style={{width: width? Number(width): undefined,
height: height? Number(height): undefined}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you are limiting width/height to numeric values only? They cannot use '100px' or 'calc(100% - 20px)'?

@cwang2016
Copy link
Contributor Author

Regarding the x log issue, it is found that no data is returned from the request (cmd: tableSearch and request for HistogramProcessor) sent to the server side. It needs more investigation to find out the reason that causes the problem. I will put this issue into another ticket.

@tgoldina
Copy link
Contributor

tgoldina commented Apr 6, 2017

I think, Histogram X log scale got broken after https://jira.lsstcorp.org/browse/DM-9342, when we added bin width option. When X log is requested, bin width and the related options should be disabled or all the values should be logs. @ejoliet needs to check it out.

@ejoliet
Copy link
Contributor

ejoliet commented Apr 6, 2017 •
edited
Loading

@tgoldina good point, i'll check with IRSA what is the default behavior to follow if log is enabled. It seems anyway that Log in XY plot is not working properly either ( David Ciardi raised the issue in IRSA jira ticket IRSA-316).

DM-9943 some fixings based on the review and update the left margin (…
f5cd445 
…the side with y axis title) based on the tick string length dynamically.
@cwang2016
Copy link
Contributor Author

cwang2016 commented Apr 7, 2017 •
edited
Loading

Updates per review comment,

  • Histogram.jsx will automatically switch to HistogramPlotly or HistogramHichart based on the chart engine setting.
  • Add plotly click event handler to resolve the histogram chart selection issue.
  • update the left/right and top/bottom margins based on the setting of x and y axis.

Additional update:

  • Update the left margin (or right margin if the yaxis title on the right side of the chart) dynamically based on the width of the text string for y ticks.

DM-9943 add mouse click event handler to the div enclosing the histog…
ce5fd8d 
…ram to solve the chart panel selection issue.
@cwang2016 cwang2016 merged commit ad34ce2 into dev Apr 9, 2017
@cwang2016 cwang2016 deleted the DM-9943-plotlyhistogram branch April 9, 2017 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loitly loitly loitly approved these changes

@tgoldina tgoldina tgoldina approved these changes

Assignees

@cwang2016 cwang2016

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cwang2016 @tgoldina @ejoliet @loitly