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

Mxmn from m4 #342

Merged
merged 16 commits into from
Jun 28, 2022
Merged

Mxmn from m4 #342

merged 16 commits into from
Jun 28, 2022

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Jun 26, 2022

Draft Working POC for an idea I had while pondering #325.

The idea is simple: pull total input yrange max and min from the m4 output since it's already computing the values per pixel window and we just need to do one more max min each step and then return the summary values with the output.


MAIN THINGS TO TEST:

  • pop a chart on this branch and one that is based on master's code ideally on the same symbol with more then 2 years worth of 1m OHLC
  • check if one feels faster then the other
  • do the same with an added --profile 10 to the chart cmd and see if you notice any further difference and/or worse numbers from the actual profiler output

Notes:

  • right now this seems to be most effective on long term crypto (1m) charts and slower (read not as fast) on 1s feeds (due to the large zeroed arrays allocated for trade rates / dark vlm?) now fixed via c620517 which means even zero-ed series are cached correctly and we get the same during-downsample-mxmn speedup effects 😎
  • obviously the mxmn summary valules are computed with a flow is not being downsampled so there's still room for improvement presuming using numba or a streaming algo (like demire see Faster y-range sorting #325) would do better for zoomed in / source data graphics in view cases.
  • the whole .maxmin() callback flow i still find super confusing, mostly because we have fsps on subcharts which each have flows but which also are somewhat needlessly calling ChartPlotWidget.maxmin() for each overlayed flow in a PlotItem
    • ideally we can somehow simplify this whole ChartView._maxmin(), ._set_yrange(), Flow.maxmin() orchestration to be more "top level triggered" by whatever view is in use and then not have Qt signal callbacks firing all over the place? needs more thought and definitely a rework to make it more understandable (unfortunately this is growing pains of figuring out how to make Qt's signals wackiness fit with our async engine..)
    • made a follow up issue to begin digging into solutions for this mess in Fixing the convoluted ._set_yrange() callbacks mess #343

@goodboy goodboy added viz graphics (charting related) geometry chops perf efficiency and latency optimization labels Jun 26, 2022
@goodboy goodboy requested review from guilledk, wygud and iamzoltan June 26, 2022 14:53
@goodboy goodboy marked this pull request as draft June 26, 2022 15:01
@goodboy goodboy self-assigned this Jun 26, 2022
@goodboy goodboy marked this pull request as ready for review June 28, 2022 00:02
y_out[bincount, 3] = y
mx = max(mx, ymx)
Copy link
Contributor Author

@goodboy goodboy Jun 28, 2022

Choose a reason for hiding this comment

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

Yeah so just to point to the crux of it all, there's really no point in not capturing these summary max/min values during each downsample cycle and this is now what we take an use in place of "manual" y-range sorting on the source data when possible.

chart.increment_view(steps=i_diff)
# chart.increment_view(steps=i_diff + round(append_diff - uppx))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

left this is in because to me the logic makes sense but for whatever reason it doesn't seem to actually be correct based on testing.

Ideally we can get to where there's never an error margin on shifts when optimizing for uppx scaling.

@@ -416,6 +417,10 @@ def maxmin(
if not slice_view.size:
mxmn = None

elif self.yrange:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bit where we choose to use the m4 yrange output if set instead of the normal "manual" sorting calls from prior.

piker/ui/_fsp.py Outdated
@@ -776,12 +781,16 @@ def chart_curves(

) -> None:
for name in names:

render = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh this can be dropped..

goodboy added 2 commits June 28, 2022 09:43
Add `ChartPlotWidget._on_screen: bool` which allows detecting for the
first state where there is y-range-able flow data loaded and able to be
drawn. Check for this flag to be set in `.maxmin()` such that until the
historical data is loaded `.default_view()` will be called to ensure
that a blank view is never shown: race with the UI starting versus the
data layer loading flow graphics can have this outcome.
f"{flow_key} no mxmn for bars_range => {key} !?"
)
res = 0, 0
if not self._on_screen:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a hitch to avoid blank views on first datums load..

probably should do something more elegant for this eventually?

@goodboy goodboy merged commit b2d5892 into master Jun 28, 2022
@goodboy goodboy deleted the mxmn_from_m4 branch June 28, 2022 14:07
@goodboy goodboy mentioned this pull request Nov 10, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphics (charting related) geometry chops perf efficiency and latency optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants