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

Speedup 01 Quicker grouping of templates #524

Merged

Conversation

flixha
Copy link
Collaborator

@flixha flixha commented Dec 12, 2022

What does this PR do?

Gives 50x speed up for grouping templates.

Why was it initiated? Any relevant Issues?

Grouping many templates was slow because the function was using a double loop for template-parameter comparison (O^2). So grouping took ~20 s for ~2000 templates. Sped up single loop to <0.5 s.

The new, quick function now circumvents the repeated calls to template.same_processing that contribute to the slowdown. So if we change what template.same_processing does (i.e., which dict-entries it compares), then we'd also need to change it here. Left in the old function for testing for now.

This PR contributes to the summary issue in #522

PR Checklist

  • develop base branch selected?
  • This PR is not directly related to an existing issue (which has no PR yet).
  • All tests still pass.
    - [ ] Any new features or fixed regressions are be covered via new tests.
    - [ ] Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGES.md.
    - [ ] First time contributors have added your name to CONTRIBUTORS.md.

@flixha flixha changed the title Speedup 01 group templates Speedup 01 Quicker grouping of templates Dec 12, 2022
@flixha
Copy link
Collaborator Author

flixha commented Dec 12, 2022

NCEDC - FDSN not answering properly in tests..

Copy link
Member

@calum-chamberlain calum-chamberlain left a comment

Choose a reason for hiding this comment

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

Looks great to me! thanks for this.

@@ -586,6 +586,19 @@ def test_tribe_copy(self):
self.assertEqual(t, copy_t)
self.assertEqual(tribe, copied)

def test_tribe_copy_quick(self):
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the wrong test for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ups.. thanks for removing!

@calum-chamberlain
Copy link
Member

Looks good to me @flixha - point noted on the template.same_processing. How about we make a template._processing_parameters property and use that instead of the:

if key in ["name", "st", "prepick", "event", "template_info"]:
    continue
else:
    actually compare

if logic in template.same_processing? Then your processing_tuples could just access those defined params. Any changes to the params compared in template.same_processing would then have to be made to the property and hence to your func?

@flixha
Copy link
Collaborator Author

flixha commented Jan 3, 2023

template._processing_parameters property and use that instead of the:

Very good suggestion. Now I defined the _processing_parameters as function / property for a Template, which helps to simplify the code at three places so that it's simpler to update any changes to what we consider processing parameters.

let me know if this is not as you were suggesting..

Copy link
Member

@calum-chamberlain calum-chamberlain left a comment

Choose a reason for hiding this comment

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

Nice, that is what I was thinking, glad you agree. Looks like that should be simpler to maintain.

@calum-chamberlain calum-chamberlain merged commit 9dafb40 into eqcorrscan:develop Jan 3, 2023
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