[FEATURE] [needs-docs] Classify symmetric #6120

pierreloicq · 2018-01-20T23:47:32Z

Hello,

The goal of this PR is to have symmetric colors and hence classes around a value. It works for equal interval, s.d. method and pretty breaks.
For equal interval and s.d. methods, there is box for the user to put the pivot value. For this, I do not manage to remove the clear button of QgsdoubleSpinBox although ShowClearButton returns false.
For pretty breaks, the pivot value is one of the pretty breaks.

pierreloicq · 2018-01-21T09:42:32Z

All checks have passed ?? Alleluiaaaaa :-)

pierreloicq · 2018-03-15T08:46:09Z

@Gustry Would you like to take a look ?

Gustry · 2018-03-15T09:05:50Z

I will try your PR later. But I can't review C++ yet ;-)
I already needed this function before I think.

You will need to update your * \since QGIS 3.0 with * \since QGIS 3.2, hopefully, it will land in QGIS this time.

Gustry · 2018-03-15T16:45:18Z

Can you also update your branch? Some of your issues with QgsdoubleSpinBox might be fixed. Better to try your PR with a fresh master branch. Thanks

pierreloicq · 2018-03-18T12:33:16Z

So I did:
Update of my master:
git checkout master
git pull upstream master --rebase
git push
rebase classifySummetric and send on github:
git checkout classifySymmetric
git rebase master
git push

change "since 3.0" to "since 3.2", git commit
now if I git push, it tells me:

To https://github.com/pierreloicq/QGIS
! [rejected] classifySymmetric -> classifySymmetric (non-fast-forward)
error: impossible de pousser des références vers 'https://github.com/pierreloicq/QGIS'
astuce: Les mises à jour ont été rejetées car la pointe de la branche courante est derrière
astuce: son homologue distant. Intégrez les changements distants (par exemple 'git pull ...')
astuce: avant de pousser à nouveau.

git branch --vv gives : " 15 behind master, 1265 ahead"
while the distant branch is: 1249 behind master, 15 ahead
1265-1249=15+1
I don't really understand why it tells me that. Is it because I rebased, git does not make the link between the 15 commits ? Did I do something wrong that I need to change to avoid this ?
I could duplicate the branch to make a new one but its a bit messy.
Can I push -f (because I am working alone on this branch )?

Thank you

DelazJ · 2018-03-18T16:40:20Z

Yes you should git push -f because your local and origin branches have diverged (none is the child of the other); afaik it does not have to do with working alone (actually you are not working alone, you have all QGIS team with you in master) but to the rebase action.
And i'm not expert in git but i do not think you made anything wrong, just missed the last bits.

Gustry · 2018-03-18T17:07:34Z

If you use the rebase command, yes you have to force push your branch. With rebase, you "re-played" the history of your commits, that you got the error "non-fast-forward".

pierreloicq · 2018-03-20T18:58:08Z

Thank you for your replies.
The warnings in the standard output when I open the layer property window disappeared and the QgsdoubleSpinBox behavior seems ok although the clear button is still there.

A complementary question :
When I do :

git checkout master
git pull upstream master --rebase
git push
git checkout classifySymmetric
git rebase master
git push

the files are changed and after that, the compilation lasts 2h. That's why I would like to avoid checkout (maybe it will not change anything?). Is it possible to update master without checkout ? Something like :

while on my working branch
git pull upstream master --rebase
git push

Gustry · 2018-03-24T08:40:45Z

As soon as you update the branch with master, it will take time to re-compile. It depends how old your branch was and files that have been updated in master.
To speed up the compilation, you need to setup ccache and ninja, check in the readme https://github.com/qgis/QGIS/blob/master/INSTALL

pierreloicq · 2018-03-25T07:47:11Z

Yes, it is already installed.
it is written :

Switching git branches will again cause longer initial build times unless
separate build directories are used for each branch.

I have only 1 build directory but I never try to compile master.

Gustry · 2018-04-19T00:10:34Z

As soon as you rebase on master, the build will be longer, because it needs to recompile the branch.

Can you include some description and new screenshots in the PR? I know it was in the previous PR, but it will be easier for reviewers to see the UI from this PR.

I gave it it a try today, it was working fine.
Can we have some advice from others who commented on previous PR? @anitagraser @andreasneumann @kannes ? #5161 and #5370

pierreloicq · 2018-04-22T14:04:26Z

Thank you Etienne, I put the description in my first post

kannes · 2018-04-24T07:13:12Z

This is awesome(!) but I see some problems which I will detail later today.

One that is easy to explain is the width the Styling Panel gets with this. Hit F7 to show it and you will see. I guess the elements should refloat to a vertical layout if needed.

kannes · 2018-04-24T18:42:59Z

I used ne_10m_admin_1_states_provinces.shp and its attribute OBJECTID_1 for testing.

a) First of all, a screenshot of the layout being too wide for the side-panel:

b) I was going to suggest using the median or average of the selected column instead of 1.00 as initial value. BUT then I realised this might entice people to use divergent color schemes without thinking just because they fit that symmetry. So I am not sure. I was confused getting several 1.00-1.00 classes at my first try. Hm...

c and d were QGIS bugs.

e) QGIS crashes shortly after I set Pretty Breaks to a single class.

pierreloicq · 2018-05-10T10:46:45Z

Hello,
I just corrected these 2 problems.
Because of the conflict noticed by github, I updated master but when I want to rebase classifySymmetric, I again have a lot of conflicts on old commits. Is there any way to bypass this ?
Thank you

pierreloicq · 2018-05-10T11:00:54Z

This small conflict can be resolved online, but for the moment, I don't dare to click on the "commit merge" button because it messed up my things last time.

DelazJ · 2018-05-10T13:52:58Z

Hi @pierreloicq
Maybe you can also squash your commits to a smaller number. I'm not sure all these commits messages (duplicate message, conflict fix) are relevant so a git rebase -i HEAD~17 leading to the necessary commit messages could be of help: less commits (changing the same lines) would reduce the number of time you'd likely hit conflict when rebasing onto master. Hth

pierreloicq · 2018-05-13T18:01:39Z

Thank you @DelazJ, it worked fine.
I finally improved the problem b) by using the mid-range value as default.

Gustry · 2018-05-13T23:53:45Z

I just gave a quick look on the files. You need to remove the file ending by .sip and keep .sip.in. It's a duplicate.

Do you mean that all issues from @kannes are fixed?

pierreloicq · 2018-05-14T15:27:42Z

Yes, all issues form @kannes are fixed.
I removed the .sip

Gustry · 2018-05-14T16:42:49Z

Another SIP issue due to #6986
You need to update your branch again.

pierreloicq · 2018-05-14T20:06:38Z

Done !

pierreloicq · 2018-09-04T10:54:14Z

Now I have a qgsgraduatedsymbolrenderer.sip generated during the compilation but none in my source. And the compilator says :
sip: core/auto_generated/symbology/qgsgraduatedsymbolrenderer.sip:333: syntax error
line 333 is:
QStringList listForCboPrettyBreaks = {},
What can I do ? @nyalldawson @Gustry
My master is up to date.

m-kuhn · 2018-09-04T11:31:51Z

Looks like a problem that sipify doesn't understand list initialization.
There are two possibilities:

Fix sipify
Replace {} with QStringList()

pierreloicq · 2018-09-04T14:38:20Z

Thank you Matthias, it works.
Is there a way to run a specific test instead of all (with "ninja test") ?

m-kuhn · 2018-09-04T14:43:03Z

Yes
ctest -R Graduated or any other regex that matches your test use ctest -V -R Graduated if you need to see more output.

Gustry · 2018-09-04T14:48:41Z

If it's a c++ test and if you use QtCreator, instead of choosing qgis, choose qgis_graduated......

So you can use the debugger

pierreloicq · 2018-09-04T15:52:45Z

Good ! I try to test my function _makeBreaksSymmetric but this is not a class function and it does not seem to be taken into account during the test. I pushed so that you can take a look. Thanks

Gustry · 2018-09-04T16:31:35Z

tests/src/core/testqgsgraduatedsymbolrenderer.cpp

@@ -31,6 +35,7 @@ class TestQgsGraduatedSymbolRenderer: public QObject
    void cleanup();// will be called after every testfunction.
    void rangesOverlap();
    void rangesHaveGaps();
+    void _makeBreaksSymmetric(QList<double> &breaks, double symmetryPoint, bool astride);


Shouldn't you call _makeBreaksSymmetric without arguments?

This is a list of function that the test will call

no the 3 functions are linked, if I want to call it line 152, arguments are needed (compilator complains)

m-kuhn · 2018-09-04T17:12:12Z

tests/src/core/testqgsgraduatedsymbolrenderer.cpp

+
+  QVERIFY( breaks.contains( symmetryPoint) );
+  // /!\ breaks contain the maximum of the distrib but not the minimum ?
+  QVERIFY( breaks.count() % 2 == 0 ); 


instead of using == use QCOMPARE

m-kuhn · 2018-09-04T17:13:07Z

tests/src/core/testqgsgraduatedsymbolrenderer.cpp

+  breaks = unchanged_breaks;
+  symmetryPoint = 12.0;
+  astride = false;
+  _makeBreaksSymmetric( breaks, symmetryPoint, astride );


I'm a bit puzzled by this call. Will this not create an infinite recursion on this method?

I don't understand how I am supposed to apply my function then.
I tried lot of things, its the only version that compiles.
Am I supposed to declare my parameters in init() ? Maybe with a pointer so that I can access them after without parameters ?

I guess you want a test function (without parameters) that calls this function with appropriate parameters?

I don't know. Something that works will be enough.
Maybe I am obliged to make _makeBreaksSymmetric() a class function ?

pierreloicq · 2018-09-05T18:25:17Z

@kannes you asked me

It would be great if changes to the "around"-value would immediately update the map (without hitting enter or another thing). This is useful if one uses the mouse wheel to change the values for immediate feedback.

I can do that by using the valueChanged signal instead of editingFinished, but then I cannot type the number with keyboard, it's always erased by the initial value, and I didn't find a way to bypass this so I put back the editingFinished.

pierreloicq · 2018-09-06T22:11:39Z

Here are the tests. They are not successful if the values to classify are below something like 1E-6. Is it expected that, using doubles, 2.6E-06 - 2E-06 = 0 ?

Gustry · 2018-09-12T11:03:11Z

Nice work about your unittest. It seems you could manage it.
Dev, is-it good from the C++ side?
I can try to compile it

Gustry · 2018-09-14T02:28:54Z

I could try the PR. Looks good! No crash. @kannes did a review already.
I don't have the best use case tonight. But as this PR is working only when we enable the checkbox "symmetric around", this looks self-contained regarding this new label "FreakingLastMinutePreFreezeMergeCandidate" ;-)

Both are more future-proof

These are advanced settings which we don't want to show up front, at risk of hurting UI simplicity

nyalldawson · 2018-09-14T04:51:22Z

@pierreloicq I just pushed a couple of last minute fixes/tweaks. Just some minor code conventions (use of switch statements when comparing enums, and hiding some private api from the python bindings -- we don't want single use methods exposed as they become subject to the stable API contract, and we may want to tweak the ui in future and need to modify these methods).

I've also reworked the UI so that it works nicely in the docked widget mode (everything needs to be nice and narrow for that to work).

Let's see what Travis says... if this passes, I'll merge for 3.4!

Thanks for all your hard work here 🎉

nyalldawson · 2018-09-14T06:36:27Z

...and in for 3.4! Thanks again @pierreloicq !

pierreloicq · 2018-09-14T20:42:20Z

Thank you guys, I'm so happy :-) :-)

nyalldawson · 2018-09-14T21:08:14Z

Better send in a follow up PR, adding yourself to the contributors map! ;)

pierreloicq mentioned this pull request Jan 21, 2018

[FEATURE] [needs-docs] classify symmetric #5370

Closed

pierreloicq changed the title ~~Classify symmetric~~ [FEATURE] [needs-docs] Classify symmetric Jan 21, 2018

nyalldawson added 3.2 Feature labels Jan 21, 2018

pierreloicq force-pushed the classifySymmetric branch from 2f58592 to 7505668 Compare March 20, 2018 18:49

pierreloicq force-pushed the classifySymmetric branch 2 times, most recently from bc5a430 to 1cb26a8 Compare May 13, 2018 15:03

pierreloicq force-pushed the classifySymmetric branch from 1d0bda0 to 8761ad6 Compare May 14, 2018 19:35

pierreloicq added 3 commits September 4, 2018 11:51

sipification

38d4e0c

2 improvements regarding kannes comments

c6907a9

Nyall's comments

04c097d

pierreloicq force-pushed the classifySymmetric branch from 4d7816f to 04c097d Compare September 4, 2018 09:51

sipification problem and unit test attempt

c7eb380

Gustry reviewed Sep 4, 2018

View reviewed changes

m-kuhn reviewed Sep 4, 2018

View reviewed changes

test classifySymmetri and changes to pass them; + editingFinished signal

238f932

sip, doc, bad keywords

38e5990

nyalldawson added the FreakingLastMinutePreFreezeMergeCandidate! label Sep 14, 2018

nyalldawson added 3 commits September 14, 2018 14:22

Mark some API as non-stable, hide from Python bindings

d4514c5

Use switches for enums, combobox data instead of index positions

b5afa7f

Both are more future-proof

Move symmetric widgets into a collapsible group box

78cb885

These are advanced settings which we don't want to show up front, at risk of hurting UI simplicity

nyalldawson merged commit f2c9160 into qgis:master Sep 14, 2018

qgib mentioned this pull request May 24, 2019

Field Calculator - No update existing field option available for PostGIS layer #15471

Closed

[FEATURE] [needs-docs] Classify symmetric #6120

[FEATURE] [needs-docs] Classify symmetric #6120

Conversation

pierreloicq commented Jan 20, 2018 • edited Loading

pierreloicq commented Jan 21, 2018

pierreloicq commented Mar 15, 2018

Gustry commented Mar 15, 2018

Gustry commented Mar 15, 2018

pierreloicq commented Mar 18, 2018 • edited Loading

DelazJ commented Mar 18, 2018

Gustry commented Mar 18, 2018

pierreloicq commented Mar 20, 2018 • edited Loading

Gustry commented Mar 24, 2018

pierreloicq commented Mar 25, 2018

Gustry commented Apr 19, 2018

pierreloicq commented Apr 22, 2018

kannes commented Apr 24, 2018

kannes commented Apr 24, 2018 • edited Loading

pierreloicq commented May 10, 2018

pierreloicq commented May 10, 2018

DelazJ commented May 10, 2018

pierreloicq commented May 13, 2018

Gustry commented May 13, 2018

pierreloicq commented May 14, 2018

Gustry commented May 14, 2018

pierreloicq commented May 14, 2018

pierreloicq commented Sep 4, 2018

m-kuhn commented Sep 4, 2018

pierreloicq commented Sep 4, 2018 • edited Loading

m-kuhn commented Sep 4, 2018

Gustry commented Sep 4, 2018 • edited Loading

pierreloicq commented Sep 4, 2018 • edited Loading

Gustry Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

pierreloicq Sep 4, 2018

Choose a reason for hiding this comment

m-kuhn Sep 4, 2018

Choose a reason for hiding this comment

m-kuhn Sep 4, 2018

Choose a reason for hiding this comment

pierreloicq Sep 4, 2018

Choose a reason for hiding this comment

m-kuhn Sep 4, 2018

Choose a reason for hiding this comment

pierreloicq Sep 4, 2018

Choose a reason for hiding this comment

pierreloicq commented Sep 5, 2018 • edited Loading

pierreloicq commented Sep 6, 2018 • edited Loading

Gustry commented Sep 12, 2018

Gustry commented Sep 14, 2018

nyalldawson commented Sep 14, 2018

nyalldawson commented Sep 14, 2018

pierreloicq commented Sep 14, 2018

nyalldawson commented Sep 14, 2018

pierreloicq commented Jan 20, 2018 •

edited

Loading

pierreloicq commented Mar 18, 2018 •

edited

Loading

pierreloicq commented Mar 20, 2018 •

edited

Loading

kannes commented Apr 24, 2018 •

edited

Loading

pierreloicq commented Sep 4, 2018 •

edited

Loading

Gustry commented Sep 4, 2018 •

edited

Loading

pierreloicq commented Sep 4, 2018 •

edited

Loading

Gustry Sep 4, 2018 •

edited

Loading

pierreloicq commented Sep 5, 2018 •

edited

Loading

pierreloicq commented Sep 6, 2018 •

edited

Loading