-
Notifications
You must be signed in to change notification settings - Fork 153
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
Expose delay link manager #2225
Expose delay link manager #2225
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2225 +/- ##
==========================================
- Coverage 88.07% 88.05% -0.02%
==========================================
Files 247 247
Lines 23039 23168 +129
==========================================
+ Hits 20291 20401 +110
- Misses 2748 2767 +19
Continue to review full report at Codecov.
|
self._disable_sync_link_manager = True | ||
yield | ||
self._disable_sync_link_manager = False | ||
|
||
@contextmanager | ||
def delay_link_manager_update(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no docstring. Will this show up in API doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another PR, but I think Glue could use a "performance tips and tricks" section like astropy
. There are some things we could have done to speed things up but they are not intuitive.
@pllim - yes performance tricks would be a good idea - it's hard to anticipate all the bottlenecks in advance though and as this PR shows there was no public API before to solve this particular problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. Bonus would be an example code snippet showing the usage... but it does not block the merge of this PR.
This can be useful for other tools such as jdaviz