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

Spatial join c++ port (with follow ups) #33579

Merged
merged 12 commits into from Jan 2, 2020

Conversation

nyalldawson
Copy link
Collaborator

Supersedes #33192, implementing the final cleanups and fixes required for merge

@nyalldawson
Copy link
Collaborator Author

For reference:

2.5million point file, joined against 3500 polygon file, release build:

Old python version: 208 seconds
New c++ version: 69 seconds

Copy link
Contributor

@alexbruy alexbruy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

qgsalgorithmjoinbylocation.cpp
---------------------
begin : January 2020
copyright : (C) 2020 by
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these should not be empty

---------------------
begin : January 2020
copyright : (C) 2020 by
email :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these lines should not be empty

qgsalgorithmjoinbylocation.cpp
---------------------
begin : January 2020
copyright : (C) 2020 by Alexis Roy-Lizotte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like more something like Alex RL and alroyliz0 at gmail dot com.

@roya0045
Copy link
Contributor

roya0045 commented Jan 2, 2020

Thanks for taking over, I knew that a more experienced programmer would implement this quicker but I wanted to help. Though I'm not sure if it helped or slowed things down. :/ Nice improvements also.

@nyalldawson nyalldawson merged commit e6a83c9 into qgis:master Jan 2, 2020
@nyalldawson nyalldawson deleted the spatial_join_c branch January 2, 2020 19:25
@nirvn
Copy link
Contributor

nirvn commented Jan 3, 2020

@roya0045 , wanting to help is a fantastic state of mind, and the QGIS project has benefited from your contributions already.

My main suggestion to you moving forward: as a 2020 new year resolution, force yourself to stop using QGIS' github repository as your test ground in favor of a proper development environment on your local machine. ATM, I have to mute most of your PRs to avoid having my inbox filling up with commits messages that would never reach the repository if you would compile locally.

VM, dual boot, whatever. Make it your resolution :)

Also, keep an open state of mind when senior developers are suggesting changes. We as a project are very, very lucky to have a couple of amazingly talented C++ / Qt gurus, and their knowledge is how we can all get better.

Thanks for your contributions in 2019; I hope you can move beyond those obstacles so you can grow as a coder and contribute even more in 2020 😄

@roya0045
Copy link
Contributor

roya0045 commented Jan 3, 2020

@nirvn Thanks for the feedback, as far as VM goes yes I now have one at work and this is how I have been able to improve my PR faster, but since its the holidays I am at home and didn't have access to my VM and with limited internet I only could do things on github directly.

Though I agree that I should test things more and take more my time with things even when I think they are just a quick PR.

And you are right that the core dev of Qgis are truly experts in their craft.

Thanks for the support!

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.

None yet

4 participants