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

Series with different intervals and 'series.connect=false' causes larger interval series to be hidden #1192

Open
ciarancourtney opened this issue Apr 11, 2019 · 13 comments

Comments

@ciarancourtney
Copy link

ciarancourtney commented Apr 11, 2019

I think it should be pretty clear whats going on here, I my case I'm comparing 60sec Vs 900sec interval data, so there is only a prop for the 900 series every 15 elements of data array, so I guess with series.connect = false the interpolation algorithm treats every edge as a gap.

Would it be helpful in terms of performance to set the expected interval on each series?

@martynasma
Copy link
Collaborator

I think what you're looking for is autoGapCount:

https://www.amcharts.com/docs/v4/chart-types/xy-chart/#Gaps_over_missing_data_points

@ciarancourtney
Copy link
Author

I think what you're looking for is autoGapCount:

https://www.amcharts.com/docs/v4/chart-types/xy-chart/#Gaps_over_missing_data_points

No its a bug, each series should be treated distinctly when it comes to joining datapoints.

@martynasma
Copy link
Collaborator

autoGapCount is a series' property, so you can set it for each series individually what you consider a "gap".

@ciarancourtney
Copy link
Author

Hi @martynasma I think the root issue is that XYSeries._baseInterval is maintained as the minimal interval e.g. if I add a 3600sec interval series gaps appear fine, if I add a 900sec interval the original gaps are broken as the baseInterval changed to 900sec. In order to set autoGapCount per series I'd need to parse and normalize the baseInterval to a fixed interval, which is an extremely hacky solution to something that should work out of the box.

I think the change is to:

  • Determine a distinct interval property per XYSeries and maintain XYSeries._baseInterval as normal
    • It may to pertinent at this point to allow setting the base 'timeUnit' to a fixed unit, like seconds, does that make sense?
  • Add option to set interval per series (great if this improves performance?)

I do have a use case for COV (Change of Value) Time Series in future, but right now my series is aligned to a fixed interval and I know it AOT

@martynasma
Copy link
Collaborator

Not sure if I get the issue. Are you saying you don't want or can't to set baseInterval for your series?

@ciarancourtney
Copy link
Author

ciarancourtney commented Apr 18, 2019

Not sure if I get the issue. Are you saying you don't want or can't to set baseInterval for your series?

@martynasma This should illustrate the issue https://stackblitz.com/edit/amcharts4-issue-1192

Note I added bullets so you can see the 3600sec interval datapoints when zooming in

Semi-related #321

@martynasma
Copy link
Collaborator

So basically, what you're saying is that we should maintain baseInterval per-series not per-axis. Right?

@ciarancourtney
Copy link
Author

So basically, what you're saying is that we should maintain baseInterval per-series not per-axis. Right?

I'm not sure of the implementation, to me baseInterval suggests a common, minimum interval that all series can align to, and that each series has a native interval, in my case between 60s and 3600s . I think a mode where the interval of each series must be specified would be a good feature in terms of determinism and performance.

Is the stackblitz example clear? When you add the 3600s series after the 900s series, it doesn't appear i.e. 100% gaps

@martynasma
Copy link
Collaborator

It's clear.

Since auto-gaps are calculated based on DateAxis.baseInterval when you have only 3600 series, it is 3600s - no gaps.

When you add a series with more granular data, baseInterval becomes smaller, satisfying autoGapCount * baseInterval for each data point of 3600 series, hence no connections.

We'll need to discuss this if we can implement it without significant overhead.

@ciarancourtney
Copy link
Author

Hi @martynasma do you have a plan to resolve this issue yet?

@martynasma
Copy link
Collaborator

Not yet, sorry.

@ciarancourtney
Copy link
Author

@martynasma are you saying this is fixed?

@martynasma
Copy link
Collaborator

No, this was auto-closed. Sorry.

I'll bring it back up, and promoted to enhancement request, so that it is not auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants