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 split lines with lines #2864

Closed
wants to merge 10 commits into from

Conversation

bstroebl
Copy link

@bstroebl bstroebl commented Mar 3, 2016

No description provided.

@bstroebl
Copy link
Author

bstroebl commented Mar 3, 2016

why is the test failing again?

@sbrunner
Copy link
Contributor

sbrunner commented Mar 3, 2016

Why spaming everybody by closing and opening new pullrequest? And losing the discussion history by the way ...

@bstroebl
Copy link
Author

bstroebl commented Mar 3, 2016

Sorry for that, Matthias already pointed it out to me.

@sbrunner
Copy link
Contributor

sbrunner commented Mar 3, 2016

OK, sorry to blame you again :-)

@m-kuhn
Copy link
Member

m-kuhn commented Mar 3, 2016

why is the test failing again?

Seems the CRS in your reference files and the one produced by running the test on travis don't match.

https://travis-ci.org/qgis/QGIS/jobs/113374867#L1294

assertLayersEqual
    _TestCase.assertEqual(self, layer_expected.dataProvider().crs().authid(), layer_result.dataProvider().crs().authid())
AssertionError: u'EPSG:4326' != u''
- EPSG:4326
+ 

Plus there are some indentation problems. Best to setup a pre-commit hook to avoid that in the future:
https://github.com/m-kuhn/QGIS-Website/blob/qtcreator.rst/source/site/getinvolved/development/git.rst#setup-pre-commit-hook

@bstroebl
Copy link
Author

bstroebl commented Mar 4, 2016

Hi Matthias,
thank you for looking into this, again. The gml files in expected now are the ones produced when running the algorithm from the toolbox and directly saving them (not outputting to temporary layers).
Where are indentation problems? In the Python code or in the yaml file?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 4, 2016

In the python file, have a look at the Details link as soon as the test finished for details.

But I would really recommend setting up the pre-commit git hook as explained in the link above, it will save you a lot of time.

@bstroebl
Copy link
Author

bstroebl commented Mar 4, 2016

But I would really recommend setting up the pre-commit git hook as explained in the link above, it will save you a lot of time.

When I (manually) runprepare-commit.sh it outputs No working copy, I enabled the hook nonetheless.

@m-kuhn
Copy link
Member

m-kuhn commented Mar 4, 2016

Can you do now:

echo '' >> python/plugins/processing/algs/qgis/SplitLinesWithLines.py
git add python/plugins/processing/algs/qgis/SplitLinesWithLines.py
git commit

It should bring print a warning that there are indentation problems and correct them for you. If you re-add the file and commit, it should be all fixed and good.

@jef-n
Copy link
Member

jef-n commented Mar 4, 2016

Better follow the tip on the dash page. The symbolic link is wrong in that forked branch.

@bstroebl
Copy link
Author

bstroebl commented Mar 4, 2016

Better follow the tip on the dash page. The symbolic link is wrong in that forked branch.

I already tried with autopep (that what you mean, Jürgen?) as proposed there, which resulted in the whole Python file being marked as changed when doing git diff

@m-kuhn
Copy link
Member

m-kuhn commented Mar 4, 2016

Thanks for pointing out @jef-n
Since I hope to merge this forked branch to our developers guide soon I updated it there as well

@bstroebl
Copy link
Author

bstroebl commented Mar 4, 2016

Maybe two empty lines are not allowed?

@jef-n
Copy link
Member

jef-n commented Mar 4, 2016

@bstroebl no, the symlink was wrongly created (see also d2c1668). And you need autopep8 1.2.1 (at least 0.9.1 is not enough - which would be what debian unstable currently ships with).

@bstroebl
Copy link
Author

bstroebl commented Mar 4, 2016

ok, seems like the indentation error is fixed now (two consecutive empty lines)
Any help with the test errors appreciated

@m-kuhn
Copy link
Member

m-kuhn commented Mar 5, 2016

There is still a mismatch in the CRS

https://travis-ci.org/qgis/QGIS/jobs/113640270#L1298

@bstroebl
Copy link
Author

bstroebl commented Mar 7, 2016

There is still a mismatch in the CRS

Yes I have seen that in the test result but why? The CRS is declared in the gfs file and all features carry the CRS information, too. So the question is, why does layer_result.dataProvider().crs().authid() retun an empty string instead of EPSG:4326?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 7, 2016

This seems to be related to the .gfs file which is present in your expected data but none of the others. Was this .gfs file produced by processing or a leftover from your save-as approach?

It looks as if processing would not create this file and this leads to results end up without a properly specified CRS.

I'm still investigating if this is a processing, QGIS or test infrastructure issue.

@bstroebl
Copy link
Author

bstroebl commented Mar 7, 2016

Yeah, you are right, looks like a left over (by file date). I will remove and recreate all files in expected and try them as a new commit, ok?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 7, 2016

Yep, that would be good.

Can you also limit the coordinate comparison precision (the results I get here are slightly less precise)

    compare:
      geometry:
        precision: 7

@bstroebl
Copy link
Author

bstroebl commented Mar 7, 2016

done both, see what the test result will be

@bstroebl
Copy link
Author

bstroebl commented Mar 8, 2016

seems that the test crashes because of the proposed coordinate precision limitation(?)

@m-kuhn
Copy link
Member

m-kuhn commented Mar 8, 2016

seems that the test crashes because of the proposed coordinate precision limitation(?)

why that? to me it looks like it's still the same CRS issue...

Interesting enough a very similar commit fixed things
m-kuhn@1859de8

@bstroebl
Copy link
Author

bstroebl commented Mar 8, 2016

why that? to me it looks like it's still the same CRS issue...

because the red cross marks this particular commit but checking the test result shows you are right

Interesting enough a very similar commit fixed things

How can I improve my PR?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 9, 2016

Not sure, I'll check

@m-kuhn
Copy link
Member

m-kuhn commented Mar 10, 2016

So it seems it's failing for a good reason this time.

I have improved debug output for the tests, I'll push this soon. Meanwhile you can already work on fixing the algorithm.

Problem description is here: https://travis-ci.org/qgis/QGIS/jobs/115020339#L1316-L1320

@bstroebl
Copy link
Author

Problem description is here: https://travis-ci.org/qgis/QGIS/jobs/115020339#L1316-L1320

Hihi, that's the code you proposed the other day :-)
Still, why is there no runtime error when running the algorithm in QGIS desktop with the same input? I could check additionally
if len(aLine.asPolyline()) > 2 or (len(aLine.asPolyline()) == 2 and aLine.asPolyline()[0] != aLine.asPolyline()[1]):

@m-kuhn
Copy link
Member

m-kuhn commented Mar 10, 2016

I get an exception when I run it on QGIS desktop and when I run the tests locally

In the build folder :

ctest -R QgisAlg -V

Just push your changes to this branch, the next results should include the backtrace

@bstroebl
Copy link
Author

ah, maybe it's because I use QGIS 2.14.0

@m-kuhn
Copy link
Member

m-kuhn commented Mar 14, 2016

Manually merged

Thanks a lot for your patience

@m-kuhn m-kuhn closed this Mar 14, 2016
@bstroebl
Copy link
Author

any hint why the tests threw errors?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 14, 2016

  • indentation
  • there were still the manually added expected result files instead of the ones produced from the test

@bstroebl
Copy link
Author

  • indentation

hmm, I deleted the blank line and thought this fixed it, you mean the backslash that you deleted in d6fca7e ? Why is it not allowed?

  • there were still the manually added expected result files instead of the ones produced from the test

how can I create them with the test? Any link that explains what one has to do?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 14, 2016

hmm, I deleted the blank line and thought this fixed it, you mean the backslash that you deleted in d6fca7e ? Why is it not allowed?

No idea, autopep8 deleted it here and is happy now

how can I create them with the test? Any link that explains what one has to do?

https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/README.md

@bstroebl
Copy link
Author

the README says

Run the algorithm you want to test in QGIS from the processing toolbox.

That's what I did and it is is different from

result files ... produced from the test

I do not want to blame you (or anybody else) but as it is now I am reluctant to write more tests (and so are others as the processing tests thread in the dev ML shows). Or would you neverthelesss encourage me to write tests and produce pull requests (with probaly fainling tests) so you can debug?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 14, 2016

The readme also says

Redirect output to python/plugins/processing/tests/testdata/expected

didn't you create the output somewhere else and then re-save it or copy it to this location?

Or would you nevertheless encourage me to write tests and produce pull requests (with probaly fainling tests) so you can debug?

As I also said on the thread in the ML: absolutely!

I am glad you opened this pull request. As a result I spent quite a bit of time in improving the test infrastructure and now there's better debug output if a test fails.

@bstroebl
Copy link
Author

didn't you create the output somewhere else and then re-save it or copy it to this location?

No (if I recall correctly) at least in the end I clicked the three-dots button and directly saved the result to testdata/expected as gml

As I also said on the thread in the ML: absolutely!

ok, so I will probably add some more tests (at least for the other algorithm I provided)

I am glad you opened this pull request. As a result I spent quite a bit of time in improving the test infrastructure and now there's better debug output if a test fails.

So how do I proceed with this PR? Shall I delete the branch or leave it as it is?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 14, 2016

No (if I recall correctly) at least in the end I clicked the three-dots button and directly saved the result to testdata/expected as gml

Strange, because here it does not create a .gfs file in this case but it does create a .xsd file - not sure if that's actually better but at least it's what succeeds on travis.

ok, so I will probably add some more tests (at least for the other algorithm I provided)

👍

So how do I proceed with this PR? Shall I delete the branch or leave it as it is?

The PR is merged and closed, so feel free to delete your branch.

@bstroebl
Copy link
Author

Strange, because here it does not create a .gfs file in this case but it does create a .xsd file - not sure if that's actually better but at least it's what succeeds on travis.

OK, will see what the next test does

@bstroebl
Copy link
Author

ok, next PR is ont its way

@bstroebl bstroebl deleted the improveSplitLinesWithLines branch March 14, 2016 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants