Skip to content

DM-8005 Create phase folding parameter setting panel and move the phase folding algorithm to the client side #220

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

Merged
merged 4 commits into from
Nov 7, 2016

Conversation

cwang2016
Copy link
Contributor

For phase folding parameter setting panel:

  • Move phase folding algorithm to the client side to create phase folded table.
  • create phase folding parameter setting panel (LcPhaseFoldingPopup.jsx)
  • adapt a React slider to create slider component (RangeSlider.jsx) for setting the period, connect the slider to FieldGroup components.
  • the existing phase folded table replaces the existing one when the complete button on the parameter setting panel is clicked.

Make client-only TableMode to work like one from the server (done by Loi),

  • table is dispatched into flux creating similar table actions as server-supported table.
  • sorting, filtering, and selected columns.
  • filter by selected rows via 'ROWID'
  • save/download table
  • convert TableCntlr to new api

Note:

  • the xyplot is made to show the data in 2 phase cycle.
  • 'period' value is set by either changing the slider or entering the data in phase folding, the valid period range and step for the slider is calculated as
    period minimum: 1 sec, i.e 1/86400 day.
    period maximum: (time_max - time_min) * 2, where time_max and time_min are respectively the maximum and minimum mjd from the raw table.
    step: (period maximum - period minimum) / (total data points * N), where N = 10.
    If a period value bigger than the maximum value on the slider is entered into the text field, then the maximum value on the slider is updated accordingly.

Some issues:

  • the xyplot area only shows the graph the first time the phase folding panel OK button is clicked. After that, only the phase folded table gets updated, and the xyplot area shows the spinning sign.
  • the cell with 'null' string as seen from the raw table is shown as blank in the phase folded table due to no null string information and processing included in the uploaded file containing the phase folded table (ipac format) and at the server side currently.

…se folding algorithm to the client side.

        Make client-only TableMode works like one from the server.
@cwang2016
Copy link
Contributor Author

Test:

  • localhost:8080/firefly/lc.html
  • load raw table 'lc_raw.tbl' -> click 'search'
  • select 'upload/phase folding' tab and click 'phase folding' button (the phase folding panel is shown)
  • play with the slider to set the period value and set other parameters
  • click 'phase folding table' to update the phase folded table which has 'phase' column added and xyplot with about 2 phase cycles is show.

@robyww
Copy link
Contributor

robyww commented Nov 4, 2016

UI Note: I think the slider CSS needs a little work we should talk about how it should look. Maybe that should not hold up this ticket but in the next week or so You, I and Loi could talk about the look. For example I played with making the slide a triangle using techniques from: https://css-tricks.com/snippets/css/css-triangle/

Copy link
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

Look goods. Two concerns are break up LcPhaseFoldingPopup.jsx and work on the UI look of the slider.

It will look really good when we get it hooked up to a chart.

@@ -0,0 +1,623 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to break out some of the non-UI function into a separate file. A large percent of this file is not UI. It will be easier to write test.

We are not always consistent about this but I think it is better to try to keep mostly only jsx in a jsx file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good approach. I am going to break it.

fieldKey: fKeyDef.period.fkey,
value: `${min}`,
label: `${fKeyDef.period.label}:`,
validator: periodValidator.bind(null, 0.0, Number.MAX_VALUE, 8, periodErr),
Copy link
Contributor

Choose a reason for hiding this comment

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

why does range slider need a validator? I would think it only allows valid values.

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 mainly for checking the value entered in the text field, and the value is applied to the slider itself.

</div>
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the flexibility of this component.

@ejoliet
Copy link
Contributor

ejoliet commented Nov 4, 2016

Thanks.
It's a good start and good demo of client new interaction!
I think there is still room to improve the slider adding 'ticks' and labels for example, and also in the choice of the default minimum and maximum values, not sure about them. Should be validated.
Adding the chart on the right-hand side will complete the feature but it will need to be as clear and useful as the previous version of regular xy plot.
Overall is fine for a demo of client capability but the feature will need to go to a round of reviews to get it finally released.
Review done.

@tgoldina
Copy link
Contributor

tgoldina commented Nov 4, 2016

Please, check http://localhost:8080/firefly catalog search. The xyplot does not show on the first catalog search, no matter IRSA or LSST. It shows on the subsequent searches. (dev branch does not exhibit this behavior).

This is my fault. I allow TBL_RESULTS_ACTIVE to be dispatched sooner, so that a tab will switch to the latest search immediately instead of waiting for table data to be completely loaded. I've reverted the code. We can talk about it when there is a need to implement such feature.

Cindy Wang and others added 3 commits November 4, 2016 17:40
1. synchronizing the default chart viewer with the active table is moved to a separate saga, this synchronization happens on CHART_ADD and on TBL_RESULTS_ACTIVE;
2. when a chart is replaced, 'mounted' attribute should be that of the previous chart.
3. the chart area did not show in Firefly Viewer (on the first catalog search).
@cwang2016 cwang2016 merged commit 9df077a into dev Nov 7, 2016
@cwang2016 cwang2016 deleted the DM-8005-PhaseFolding branch November 8, 2016 01:05
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