Refactor graduated symbol renderer #31500
Conversation
3nids
force-pushed
the
refactor_classification
branch
from
2d1f8e2
to
68b1746
Compare
|
Missing bits were completed and this is now tested. The only changes for the user:
|
an abstract class for classification methods
use an editable combobox instead of a spin or combobox
3nids
force-pushed
the
refactor_classification
branch
from
a45cd7f
to
09497b0
Compare
3nids
force-pushed
the
refactor_classification
branch
4 times, most recently
from
7b3b2c9
to
b66b839
Compare
3nids
force-pushed
the
refactor_classification
branch
from
b66b839
to
b6278ae
Compare
3nids
force-pushed
the
refactor_classification
branch
from
b38549a
to
f55dd95
Compare
3nids
force-pushed
the
refactor_classification
branch
from
718e01e
to
d10f8ce
Compare
|
The PR is ready for review. Last missing thing to get green light from Travis is a warning when building the API docs probably due to use of Regarding the review, I have mainly one point I'd like opinions. It's in the graduated symbol renderer which keeps a pointer to the method. A new method can be set using a setter. |
| breaks << value; | ||
| } | ||
| breaks[nclasses - 1] = maximum; | ||
| } |
There was a problem hiding this comment.
For review purposes -- was any of the actual logic changed in these?
There was a problem hiding this comment.
no.
The only thing I find weird is that breaks always include maximum but not minimum values. I kept this behavior.
| // Jenks Optimal (Natural Breaks) algorithm | ||
| // Based on the Jenks algorithm from the 'classInt' package available for | ||
| // the R statistical prgramming language, and from Python code from here: | ||
| // http://danieljlewis.org/2010/06/07/jenks-natural-breaks-algorithm-in-python/ |
There was a problem hiding this comment.
note to self - we should resync this with classInt and double check we aren't missing any upstream fixes...
| // if we have lots of values, we need to take a random sample | ||
| if ( values.size() > mMaximumSize ) | ||
| { | ||
| // for now, sample at least maximumSize values or a 10% sample, whichever |
There was a problem hiding this comment.
is it worth moving the support for max sample sizes outside of this, so that the feature request used to fetch the values has a setLimit on it?
There was a problem hiding this comment.
That makes sense but a beyond the scope of this PR. I believe this can be easily added later on, even in classification base class.
src/core/symbology/classification/qgsclassificationmethodregistry.h
Outdated
Show resolved
Hide resolved
src/core/symbology/classification/qgsclassificationmethodregistry.h
Outdated
Show resolved
Hide resolved
3nids
force-pushed
the
refactor_classification
branch
2 times, most recently
from
fd6a734
to
5b9fc1f
Compare
3nids
force-pushed
the
refactor_classification
branch
5 times, most recently
from
fe2a454
to
2fae425
Compare
3nids
force-pushed
the
refactor_classification
branch
from
2fae425
to
9a08913
Compare
|
@3nids a minor regression here:
(duplicate "std dev" text) |
This is the implementation of qgis/QGIS-Enhancement-Proposals#147
Some work remains to be done in the widget and reading/writing to XML.