-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add support for running batch on a single lightcurve #420
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 95.82% 95.84% +0.02%
==========================================
Files 25 25
Lines 1771 1781 +10
==========================================
+ Hits 1697 1707 +10
Misses 74 74 ☔ View full report in Codecov by Sentry. |
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.
This looks good, just have one main change that I think should be made.
src/tape/ensemble.py
Outdated
@@ -1107,6 +1108,9 @@ def batch( | |||
source or object tables. If not specified, then the id column is | |||
used by default. For TAPE and `light-curve` functions this is | |||
populated automatically. | |||
single_lc: `int`, optional |
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.
I think it would be really helpful to also allow single_lc=True
, where select_random_lightcurve is used to pick one for the user.
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.
Yeah that makes sense. To do that, I added an id_only
parameter to Ensemble.select_random_timeseries
so that it will only fetch the object id of a random lightcurve. But let me know if you would prefer that not added to the API and I can break that out differently.
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.
Thanks! That looks like a good solution
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.
Looks good!
Change Description
In #409 it was proposed to add a batch helper function
try_batch
which would allowEnsemble.batch
to be tested on a single lightcurve. To accomplish that goal, here we instead add a new parametersingle_lc
toEnsemble.batch
When
single_lc
is provided an object ID, the function provided tobatch
is executed on that single lightcurve.Code Quality
New Feature Checklist