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

remove o(n^2) operation from NumberedObjectCollection #563

Merged

Conversation

MicahGale
Copy link
Collaborator

@MicahGale MicahGale commented Oct 2, 2024

Description

As described in #556 there was an O(N^2) operation for appending N items to a NumberedObjectCollection. This was because self.numbers (which is an O(N) generator) was checked every append. This was fixed by:

  1. Moving all number conflict checks to the existing check_number
  2. Trusting __num_cache explicitly iff this problem is linked to a problem.
  3. creating a method: _update_number that updates __num_cache appropriately when a number is changed.
  4. Ensuring all Numbered_MCNPObject instances run _update_number if they are attached a problem. This is why __num_cache is trusted in this case.

This doesn't remove all bottlenecks and get us to O(N) time, but it does make append no longer a bottle neck as can be seen in the profiling results.

Don't review this until #539 is merged because this is built off of that branch.

Fixes #556

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@MicahGale MicahGale linked an issue Oct 2, 2024 that may be closed by this pull request
@MicahGale MicahGale self-assigned this Oct 2, 2024
@MicahGale MicahGale added feature request An issue that improves the user interface. performance 🐌 Issues related to speed and memory labels Oct 2, 2024
@MicahGale
Copy link
Collaborator Author

This depends on #539

@MicahGale
Copy link
Collaborator Author

MicahGale commented Oct 4, 2024

There is still an O(N^2) call to .numbers:

Function                                                                                                                  was called by...
                                                                                                                              ncalls  tottime  cumtime
/home/mgale/mambaforge/envs/data/lib/python3.12/site-packages/montepy/numbered_object_collection.py:75(numbers)           <-   28498    0.018    0.022  /home/mgale/mambaforge/envs/data/lib/python3.12/site-packages/montepy/data_inputs/universe_input.py:157(push_to_cells)
                                                                                                                            25937845   18.944   95.303  /home/mgale/mambaforge/envs/data/lib/python3.12/site-packages/montepy/numbered_object_collection.py:192(append)

@MicahGale
Copy link
Collaborator Author

I can't replicate this O(N^2) behavior. I wonder if the wrong version of MontePy was used in the test.

@MicahGale
Copy link
Collaborator Author

Upon further examination this was a problem with my tests. If you look at the profile results in the CI, numbers doesn't even show up and append is very low on the list. So I think we can say this was solved.

@MicahGale MicahGale marked this pull request as ready for review October 9, 2024 13:43
@MicahGale MicahGale added the code improvement A feature request that will improve the software and its maintainability, but be invisible to users. label Oct 9, 2024
@MicahGale MicahGale mentioned this pull request Oct 9, 2024
montepy/numbered_object_collection.py Outdated Show resolved Hide resolved
montepy/numbered_object_collection.py Outdated Show resolved Hide resolved
@tjlaboss
Copy link
Collaborator

Two small changes to make.

@MicahGale MicahGale changed the title 556 remove on2 operation from numberedobjectcollection 556 remove o(n2) operation from numberedobjectcollection Oct 11, 2024
@MicahGale MicahGale changed the title 556 remove o(n2) operation from numberedobjectcollection 556 remove o(n^2) operation from numberedobjectcollection Oct 11, 2024
@MicahGale
Copy link
Collaborator Author

No change in memory usage (duh), but went from 27.6 seconds to 26.7 seconds for this improvement. This is a 3% reduction for the "big model". Though this is a low cost O(N^2) operation, and the saving should scale accordingly.

@MicahGale MicahGale changed the title 556 remove o(n^2) operation from numberedobjectcollection 556 remove o(n^2) operation from NumberedObjectCollection Oct 15, 2024
@MicahGale MicahGale changed the title 556 remove o(n^2) operation from NumberedObjectCollection remove o(n^2) operation from NumberedObjectCollection Oct 15, 2024
@MicahGale MicahGale merged commit 825592e into develop Oct 15, 2024
20 checks passed
@MicahGale MicahGale deleted the 556-remove-on2-operation-from-numberedobjectcollection branch October 15, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code improvement A feature request that will improve the software and its maintainability, but be invisible to users. feature request An issue that improves the user interface. performance 🐌 Issues related to speed and memory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove O(N^2) operation from NumberedObjectCollection
2 participants