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

Improve readability of the daily burnt BSQ chart #3890

Merged

Conversation

@dmos62
Copy link
Contributor

dmos62 commented Jan 11, 2020

Relevant issue: #3753

These commits are about improving readability of the daily burnt BSQ chart. It is done by introducing a 'Zoom to inliers' toggle, which when active zooms in on inliers, and by introducing a moving average. The zooming feature is automatic and should be at least somewhat robust to new trends. The moving average contextualizes the large peaks and gullies and generally improves interpretability of this noisy series. Also, the charts' Y axes are set to include 0, most notably in the monthly issued BSQ chart.

I have provided details in the commit messages.

The numerous changes to the SupplyView class are contained in a single commit, which I realise is not comfortable to review. Ideally I would have refactored in a separate commit, but I wasn't sure how I want to refactor until many of the changes were already implemented.

Below are screenshots illustrating changes to the daily burnt BSQ chart:

This is before changes:
bsq burnt before changes
This is a GIF slideshow (2 frames) of the zoom toggle being toggled in the new chart:
bsq burnt after changes animation

This took much longer to implement than expected. There were quite a few nuances I didn't anticipate.

Furthermore, some of the more minute cosmetic choices I made (or didn't make) might not be great, because since the holidays (and for a few weeks more) I'm using a pretty old machine, which makes the ROI of iterating less significant GUI changes prohibitvely low.

@dmos62 dmos62 requested review from ripcurlx and sqrrm as code owners Jan 11, 2020
Copy link
Member

ripcurlx left a comment

NACK - Running reformat code and optimize imports in IntelliJ changes quite a lot in your files. Please adapt our code style.

Just for functionality your code does what it is expected to do. Could you please adapt the style of the line chart so the BSQ burnt has the same line thickness as the chart above. Atm it looks still a little bit developer style 😉 .
Bildschirmfoto 2020-01-16 um 16 30 56

Thanks for this contribution 👍


package bisq.common.util;

import bisq.common.util.Tuple2;

This comment has been minimized.

Copy link
@ripcurlx

ripcurlx Jan 16, 2020

Member

Not used in code

import bisq.common.util.Tuple2;

import java.util.DoubleSummaryStatistics;
import bisq.common.util.DoubleSummaryStatisticsWithStdDev;

This comment has been minimized.

Copy link
@ripcurlx

ripcurlx Jan 16, 2020

Member

Not used in code

import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import java.util.Spliterators.AbstractSpliterator;

import java.util.function.BiFunction;
import java.util.function.Supplier;
import java.util.function.Consumer;

import javafx.collections.ListChangeListener;

import static bisq.desktop.util.FormBuilder.addTitledGroupBg;
import static bisq.desktop.util.FormBuilder.addTopLabelReadOnlyTextField;
import static bisq.desktop.util.FormBuilder.addSlideToggleButton;
Comment on lines 73 to 92

This comment has been minimized.

Copy link
@ripcurlx

ripcurlx Jan 16, 2020

Member

Do you use IntelliJ for development? If yes, please use the formatting functionality that auto sorts and groups the different imports.

This comment has been minimized.

Copy link
@dmos62

dmos62 Jan 17, 2020

Author Contributor

I use vim and I do formatting and similar tasks manually. I'll ask for suggestions on #dev.

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Jan 17, 2020

@dmos I activated yesterday codacy's PR Quality Review for the Bisq repository. It found for your PR a couple of issues which you can view in the Details link next to the PR check. Please review and push fixes for this issues to trigger a re-review by codacy. Thanks!

@sqrrm

This comment has been minimized.

Copy link
Member

sqrrm commented Jan 17, 2020

Considering the weekly activity by the burning man it's known that every 7 days there will be a spike making the moving average a bit pointless. I suggest to not include the days with extreme outliers, which will be the burning days. The activity by the burning man is not that relevant as it's going to be correlating to the amount of bugs and failed trades.

Another thought could be to also add the moving average over the outliers to get a picture of how much is burnt due to failed trades, and thus show two moving averages.

@dmos62

This comment has been minimized.

Copy link
Contributor Author

dmos62 commented Jan 17, 2020

@sqrrm if I understand correctly, your suggestion is to treat the very big spikes, like the ones on the right of the chart, as if it was a different data series. I.e. present it and the rest of daily burnt BSQ separately and analyse and reason about it separately as well.

I'm not sure I understand what the burning man is (I only know Burning Man, haha). I'll just suppose that it's something related to the 7 day cycle you mention.

So, if we were to do that, it would be much preferable to perform the "separation" earlier in the pipeline, ideally mirroring how it works with the issued BSQ series in daoStateService.getIssuanceSet(issuanceType), issuanceType being either REIMBURSEMENT or COMPENSATION.

That said, as I see it, both these processes that we possibly would want to reason about separately are highly related in that they're burning BSQ, so being able analysing their sum total effect is desirable. The question would be if we want to also analyse them in isolation of each other.

As for the moving average, if we do want to analyse burning BSQ as a whole, it implies analysing what influence the inliers and outliers have on their total effect, and for that MA is important. It not only improves the readability of the noisy inliers, but it translates outliers and inliers to a "common denominator" for the lack of a better term.

@sqrrm

This comment has been minimized.

Copy link
Member

sqrrm commented Jan 17, 2020

Yes, I consider them different data sets for most of our use cases. The problem is of course to do a proper separation, but we should in theory know exactly which coins were burnt by the burning man as this role has to report exactly what was burnt.

I don't know if it's possible to find this through the DAO, but I suspect not as there would be a hash in the opreturn but I don't think you have enough data to calculate that hash.

My suggestion was just the easiest solution to at least get some kind of usable moving average. I wouldn't spend too much time on this, it's probably more useful to manipulate this data outside of the app for any kind of analysis.

@dmos62

This comment has been minimized.

Copy link
Contributor Author

dmos62 commented Jan 17, 2020

The more I think about it, the better I like the idea of tagging the inliers and outliers as explicit subseries within burnt BSQ. That would probably require tagging them as such on burning; I haven't looked at how issued BSQ is subdivided. If we had thought of this before, the automatic computation of the inlier range would not have been necessary.

Edit: oh actually, we probably can't tag retroactively, so something like this would still be necessary for analysing data accumulated until now.

This is all good food for thought. I wonder how the new priority based compensations will influence work on this.

This PR contains functionality to separate the inliers and outliers, so we have what we need for separating the series, and in turn having separate MAs, but doing it so far downstream has a hacky feel. Though this method would stop working if the clear distinction between the series disappeared in the future.

Could you describe the use cases you have in mind in the linked issue thread? That would be very useful.

dmos62 added 3 commits Jan 18, 2020
This provides an efficient way to calculate the standard deviation from
a DoubleStream, and is to be used in measuring whether or not a data
point on a chart axis is an inlier.

The Kahan summation algorithm is in a slightly different style, because
it's a faithful copy of its implementation in JDK 11's
DoubleSummaryStatistics.
This utility provides ways to zoom in a chart (more specifically its
axes) on inlier data points. The principal use-case is to restore
readability to charts that were distorted by outliers. The first (and
maybe the last) chart to use this is the daily burnt BSQ chart under
'DAO -> Facts and Figures'.
Allows computing a simple moving average lazily from a Stream, to be
used in plotting noisy data. Will be used in the daily burnt BSQ chart
under 'DAO -> Facts and Figures'. Can be easily modified to compute
other moving averages, like exponential.
@dmos62 dmos62 force-pushed the dmos62:dao-facts-and-figures-outlier-resistance branch from 884c8cb to 1901501 Jan 18, 2020
Relevant issue thread: #3753

Currently the daily burnt BSQ chart under 'DAO -> Facts and Figures' is
distorted by outliers. This introduces a 'Zoom to inliers' toggle (off
by default), which, when toggled on, effectively zooms the chart to
inliers, thus removing the distortion. Also, a moving average is
plotted, to further improve the chart's readibility.

The chart is also changed from an area chart to a line chart, on the
presumption that it was an area chart for cosmetic reasons, but now that
there are two series in it (the moving average was added) an area chart
makes less sense.

Another noteworthy change is that the other chart in the screen, monthly
issued BSQ, has its Y axis set to start at zero, so as to improve
readability. This might seem outside the scope of this commit, but the
other changes involved some refactoring, which involved cleaning up some
duplicated logic, which involved configuring both of these charts
together, which involved forcing zero to be on the axis.

This implementation mixes some plotting logic (responsible for zooming
in on inliers) into the view logic, because I opted to implement said
zooming as an external manipulation of a chart's axis. I chose this in
favor of implementing a new Chart, because it would have required
including multiple large classes (relevant JavaFX's classes can't be
ergonomically extended) to the code base. I presumed that my chosen
solution will be easier to maintain.

I am not entirely happy with this choice and can see myself introducing
some plotting-related classes to encapsulate creating charts like these,
thus unmixing plotting logic from view logic. In the meantime this is a
working solution, and I plan to continue working on these charts in the
near future.
@dmos62 dmos62 force-pushed the dmos62:dao-facts-and-figures-outlier-resistance branch from 1901501 to 86489e0 Jan 18, 2020
@dmos62

This comment has been minimized.

Copy link
Contributor Author

dmos62 commented Jan 18, 2020

Did a forced push.

Organized imports and reformated code with IntelliJ. Made CSS changes to the chart. Namely, the plot line width was reduced to 1px, and the plot line colors were changed to mirror more closely the perceived color of the stacked area chart above and, at the same time, they were also made to adapt to the different backgrounds under light and dark modes.

bsq burnt after style changes

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Jan 22, 2020

@dmos62 I just pushed some minor code changes to get rid of as many IntelliJ warnings as possible. Please let me know if you are fine with those changes.

@dmos62

This comment has been minimized.

Copy link
Contributor Author

dmos62 commented Jan 23, 2020

@ripcurlx Yes, I'm fine with the changes. Thanks for the improvements.

@ripcurlx ripcurlx merged commit a29d490 into bisq-network:master Jan 23, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.6 milestone Jan 23, 2020
@dmos62 dmos62 deleted the dmos62:dao-facts-and-figures-outlier-resistance branch Jan 23, 2020
@dmos62 dmos62 mentioned this pull request Feb 8, 2020
@ripcurlx ripcurlx mentioned this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.