Deprecate store_aggregation and update docs - #3738
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@jaegeral can you take a look into this? |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly deprecates the store_aggregation function in favor of Aggregation.save(), adding a DeprecationWarning as is standard practice. The accompanying documentation update in the demo notebook is also appropriate, but contains a small error in the new instructions. I've left a specific comment with a suggestion to correct the notebook text to accurately guide users on how to use the new method.
| "Start by checking out whether there are any stored aggregations that we can just take a look at.\n", | ||
| "\n", | ||
| "You can also store your own aggregations using the `gd_sketch.store_aggregation` function. However we are not going to do that in this colab." | ||
| "You can also store your own aggregations using the `gd_sketch.save` function. However we are not going to do that in this colab." |
There was a problem hiding this comment.
The updated documentation is slightly incorrect. The Sketch object (gd_sketch) does not have a save method. The deprecation notice for store_aggregation correctly points to using Aggregation.save(). The text should be updated to reflect that the .save() method is on an Aggregation object, not the Sketch object.
| "You can also store your own aggregations using the `gd_sketch.save` function. However we are not going to do that in this colab." | |
| "You can also store your own aggregations by calling the `.save()` method on an aggregation object. However we are not going to do that in this colab." |
There was a problem hiding this comment.
I think it is okay to not touch the notebook.
| """ | ||
| if self.is_archived(): | ||
| raise RuntimeError("Unable to store an aggregator on an archived sketch.") | ||
| warnings.warn( |
There was a problem hiding this comment.
What did we actually gain if you raise a warning instead of a logger message? I think we would be better of to remove the method all together. Or remove the rest of the code.
|
Hey @p-nowodzinski , any thoughts on the PR? I might lean towards just removing the method tbh |
|
Hi, apologies for the delayed response and thanks for the feedback. That makes sense as I initially suggested deprecation in case you preferred to keep the function, but as it doesn’t appear to be used elsewhere, removing it would help simplify the codebase. I'm happy to proceed with removing store_aggregation(). Let me know your preference and I’ll update the PR accordingly. |
|
Yeah remove it |
20102c3 to
27143a6
Compare
3 participants
Deprecate store_aggregation() function
Added DeprecationWarning to store_aggregation() and updated notebook demo to remove a reference to calling store_aggregation()
Changed "You can also store your own aggregations using gd_sketch.store_aggregation function" to "You can also store your own aggregations using gd_sketch.save function".
Guides users to use Aggregation.save() instead