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

Electorate and State facets should be consistent with project overview results #1062

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

temi
Copy link
Contributor

@temi temi commented Feb 5, 2025

  • added new facets to support electorates and states from geographic info property and sites

 - added new facets to support electorates and states from geographic info property and sites
@temi temi requested a review from chrisala February 5, 2025 00:02
temi added 2 commits February 5, 2025 13:29
 - added test case to make sure point intersections are last
Copy link
Collaborator

@chrisala chrisala left a comment

Choose a reason for hiding this comment

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

Maybe consider renaming "autoStateFacet/autoElectFacet" as "projectStateFacet"/"projectElectFacet" as it's not auto if the user overrides the geographic info?

Given we are now displaying this every time a project is opened and static caching it in the project explorer it might be worth just saving it on the Project entity rather than recalcuating it each call. (It's called quite a lot in the logs and often takes 100-300 milliseconds to return)

@temi
Copy link
Contributor Author

temi commented Feb 6, 2025

@chrisala yes, "projectStateFacet"/"projectElectFacet" is a better name. I will rename them.

Saving this information will mean updating the project every time a new site is added/updated/deleted. Coding can get complex and could possibly have negative consequence on system performance. I will check for ways to make this faster. Let me know if there are alternatives.

@chrisala
Copy link
Collaborator

chrisala commented Feb 6, 2025

Saving this information will mean updating the project every time a new site is added/updated/deleted. Coding can get complex and could possibly have negative consequence on system performance. I will check for ways to make this faster. Let me know if there are alternatives.

My perspective at the moment is it's having a bigger impact on system performance as it is now.
e.g. today so far it's been called ~250 times with response times between 30 and 300 milliseconds
Once we add the indexing it will be called for every project / site write as well (as these trigger a project reindex)

By comparison we've had 13 site writes and 18 project writes. Project writes are responding on average in 10-30 milliseconds.

Maybe we could cache the read and invalidate it when a site is updated (and we do the re-intersect)?

@temi
Copy link
Contributor Author

temi commented Feb 6, 2025

Yes, agree this needs to be faster. Do you have the project ids for which this takes a long time?

 - renamed autoStateFacet to projectStateFacet and autoElectFacet to projectElectFacet
 - add geographic info parameters to ProjectService.toMap
@temi
Copy link
Contributor Author

temi commented Feb 6, 2025

@chrisala I have updated code as discussed.

@temi temi merged commit 7406874 into dev Feb 6, 2025
3 checks passed
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.

2 participants