Limit fixed interval classification to generate at most 999 classes #50290

uclaros · 2022-09-22T10:27:29Z

Description

Fixed interval has a default value of 1 which can generate way too many classes, leading to crashes - see #49333.
This PR automatically changes the interval size to a value that will not cause more than 999 classes.
999 is the maximum number of classes that we allow for the other interval types so I used this as a ceiling.

Editing the Interval size line edit widget can be a little klutzy when trying to change the forced minimum value, but that's a way better side effect than what we have now (potential crash).
@nyalldawson is there a more elegant way to change the parameter's default value dynamically?

Fix #49333

nyalldawson · 2022-09-25T23:34:58Z

@nyalldawson is there a more elegant way to change the parameter's default value dynamically?

I'd suggest we don't change the value at all, but instead just apply the limit internally. For bonus points we could add some error reporting capacity to calculateBreaks and then show a warning label in the gui that too many classes were generated and the classification was limited to 999 classes.

github-actions · 2022-10-12T03:23:31Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

nyalldawson · 2022-10-17T00:46:10Z

src/core/symbology/qgsgraduatedsymbolrenderer.h

     */
-    void updateClasses( const QgsVectorLayer *vl, int nclasses );
+    void updateClasses( const QgsVectorLayer *vl, int nclasses, QString *error = nullptr );


Suggested change

void updateClasses( const QgsVectorLayer *vl, int nclasses, QString *error = nullptr );

void updateClasses( const QgsVectorLayer *vl, int nclasses, QString *error SIP_OUT = nullptr );

nyalldawson · 2022-10-17T00:47:07Z

src/core/classification/qgsclassificationmethod.h

     */
-    QList<QgsClassificationRange> classes( const QgsVectorLayer *layer, const QString &expression, int nclasses );
+    QList<QgsClassificationRange> classes( const QgsVectorLayer *layer, const QString &expression, int nclasses, QString *error = nullptr );


Suggested change

QList<QgsClassificationRange> classes( const QgsVectorLayer *layer, const QString &expression, int nclasses, QString *error = nullptr );

QList<QgsClassificationRange> classes( const QgsVectorLayer *layer, const QString &expression, int nclasses, QString *error SIP_PYARGREMOVE = nullptr );

We can't add this to the bindings, or the method will start returning a tuple and break existing scripts

nyalldawson · 2022-10-17T00:47:50Z

src/core/classification/qgsclassificationmethod.h

@@ -335,7 +336,7 @@ class CORE_EXPORT QgsClassificationMethod SIP_ABSTRACT
     * The maximum value is expected to be added at the end of the list, but not the minimum
     */
    virtual QList<double> calculateBreaks( double &minimum, double &maximum,
-                                           const QList<double> &values, int nclasses ) = 0;
+                                           const QList<double> &values, int nclasses, QString *error = nullptr ) = 0;


Suggested change

const QList<double> &values, int nclasses, QString *error = nullptr ) = 0;

const QList<double> &values, int nclasses, QString *error SIP_PYARGREMOVE = nullptr ) = 0;

For the same reason as above... although, I'm unsure how sip will handle this with a virtual method 😱

nyalldawson · 2022-10-17T00:48:52Z

src/core/classification/qgsclassificationfixedinterval.cpp

+    if ( breaks.length() >= 999 )
+    {
+      error->clear();
+      error->append( QObject::tr( "The specified interval would generate too many classes.\nOnly the first 999 classes are actually added." ) );
+      break;
+    }


Suggested change

if ( breaks.length() >= 999 )

{

error->clear();

error->append( QObject::tr( "The specified interval would generate too many classes.\nOnly the first 999 classes are actually added." ) );

break;

}

if ( breaks.length() >= 999 && error )

{

error = QObject::tr( "The specified interval would generate too many classes.\nOnly the first 999 classes are actually added." );

break;

}

github-actions · 2022-11-01T03:21:36Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

github-actions · 2022-11-09T03:12:53Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

nyalldawson · 2024-06-10T03:12:54Z

Resurrected in #57722

limit fixed interval to generate at most 999 classes

e11bf57

github-actions bot added this to the 3.28.0 milestone Sep 22, 2022

uclaros added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Sep 22, 2022

fix test and handle rounding issue when using 999 classes

944a799

introduce error reporting parameter, limit classes to 999

69a1bd3

github-actions bot added the GUI/UX Related to QGIS application GUI or User Experience label Sep 27, 2022

uclaros added 3 commits September 27, 2022 16:25

fix test

7c1a0ae

update docstrings

521fead

q_unused

a4be69d

uclaros mentioned this pull request Oct 1, 2022

Crash after selecting Fixed Interval mode in graduated symbology #50416

Closed

2 tasks

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 12, 2022

nyalldawson reviewed Oct 17, 2022

View reviewed changes

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 17, 2022

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 1, 2022

github-actions bot closed this Nov 9, 2022

nyalldawson mentioned this pull request Jun 10, 2024

Limit fixed interval classification to generate at most 999 classes (resurrected) #57722

Merged

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit fixed interval classification to generate at most 999 classes #50290

Limit fixed interval classification to generate at most 999 classes #50290

uclaros commented Sep 22, 2022

nyalldawson commented Sep 25, 2022

github-actions bot commented Oct 12, 2022

nyalldawson Oct 17, 2022

nyalldawson Oct 17, 2022

nyalldawson Oct 17, 2022

nyalldawson Oct 17, 2022

github-actions bot commented Nov 1, 2022

github-actions bot commented Nov 9, 2022

nyalldawson commented Jun 10, 2024

	void updateClasses( const QgsVectorLayer vl, int nclasses, QString error = nullptr );
	void updateClasses( const QgsVectorLayer vl, int nclasses, QString error SIP_OUT = nullptr );

	QList<QgsClassificationRange> classes( const QgsVectorLayer layer, const QString &expression, int nclasses, QString error = nullptr );
	QList<QgsClassificationRange> classes( const QgsVectorLayer layer, const QString &expression, int nclasses, QString error SIP_PYARGREMOVE = nullptr );

	const QList<double> &values, int nclasses, QString *error = nullptr ) = 0;
	const QList<double> &values, int nclasses, QString *error SIP_PYARGREMOVE = nullptr ) = 0;

Limit fixed interval classification to generate at most 999 classes #50290

Limit fixed interval classification to generate at most 999 classes #50290

Conversation

uclaros commented Sep 22, 2022

Description

nyalldawson commented Sep 25, 2022

github-actions bot commented Oct 12, 2022

nyalldawson Oct 17, 2022

Choose a reason for hiding this comment

nyalldawson Oct 17, 2022

Choose a reason for hiding this comment

nyalldawson Oct 17, 2022

Choose a reason for hiding this comment

nyalldawson Oct 17, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 1, 2022

github-actions bot commented Nov 9, 2022

nyalldawson commented Jun 10, 2024