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 extract speed #965

Merged
merged 3 commits into from
Jan 9, 2020
Merged

Improve extract speed #965

merged 3 commits into from
Jan 9, 2020

Conversation

vvmruder
Copy link
Collaborator

@vvmruder vvmruder commented Dec 6, 2019

Improve municipality handling, this might break existing adapted non standard sources!!!

This is related to #823

@peterschaer
Copy link
Collaborator

Thanks! This is waaaay faster than before (on a small real estate from 3secs to 1secs for a JSON extract!). So it looks good to me.

@vvmruder vvmruder added the X breaking change X Marks breaking changes label Dec 6, 2019
@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

@peterschaer Nice that it works. Do you know if you use some adapted elements in your project which you need to change? Or was it working out of the box?

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

And in addition: Did you tried the capabilities request? The fix is interfering with this. So I would be interested if output is still like you would expect it.

@peterschaer
Copy link
Collaborator

It was working out of the box, I didn't have to change anything. however the capabilities request has different output. before it had 346 municipalities (the same number as in the municipality table), now it has 702 municipalities. there are a lot of duplicates.

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

Ups... I will have a look at it.

@vvmruder vvmruder changed the title Improve extract speed [WIP] Improve extract speed Dec 6, 2019
@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

@peterschaer Could you please try again? I hope my fix solve this issue.

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

And maybe can you try several times to see if the duplicates are increasing if the error still exists?

@pfirpfel
Copy link
Contributor

pfirpfel commented Dec 6, 2019

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- pyramid_oereb/standard/sources/municipality.py  1
         

See the complete overview on Codacy

@peterschaer
Copy link
Collaborator

very good, the duplicates are gone even if i try multiple times.

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

Juhu. I will wait for review of the others. But in my opinion we can merge this. It is quite an improvement. Even if it only really bothers cantons with really many municipalities 😃

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

@peterschaer Thanks for catching this in the code!

Copy link
Collaborator

@kdeininger kdeininger left a comment

Choose a reason for hiding this comment

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

Very nice performance improvement!

This change should be even allow backward compatibility as fosnr is an optional argument. Calling the read methods without argument, they will act the same way as they did before and even if someone has extended one of the classes, the optional argument should not cause any problems.

@vvmruder
Copy link
Collaborator Author

vvmruder commented Dec 6, 2019

@kdeininger I'am glad you see it like this. I think this too. I will wait for comment from @jwkaltz and then we can merge.

@vvmruder vvmruder changed the title [WIP] Improve extract speed Improve extract speed Dec 6, 2019
Copy link
Member

@jwkaltz jwkaltz left a comment

Choose a reason for hiding this comment

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

Thanks @vvmruder it looks good to me, feel free to merge.

Question: should we increase the version number of pyramid_oereb to 1.6, since this is a potentially breaking change? (Although I am currently not aware of anybody using non-standard sources)

Copy link
Collaborator

@voisardf voisardf left a comment

Choose a reason for hiding this comment

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

Thanks a lot, just one question not having the data flow present right now.

Returns:
list of pyramid_oereb.lib.records.municipality.MunicipalityRecord:
pyramid_oereb.lib.records.municipality.MunicipalityRecord:
The list of all found records. Since these are not filtered by any criteria the list simply
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it still a list at this point? If not, we should adapt the comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it still is a list. But this is a good point. The comment should be adapted anyway as the list may be filtered now.

@vvmruder vvmruder removed the X breaking change X Marks breaking changes label Dec 20, 2019
@vvmruder vvmruder merged commit 3d59ccf into master Jan 9, 2020
@vvmruder vvmruder deleted the improve_municipality_handling branch January 9, 2020 07:56
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.

7 participants