Support of light-curve package #201
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 92.20% 92.45% +0.24%
==========================================
Files 21 22 +1
Lines 1091 1126 +35
==========================================
+ Hits 1006 1041 +35
Misses 85 85
☔ View full report in Codecov by Sentry. |
hombit
force-pushed
the
light-curve-package
branch
from
3814574 to
150f64c
Compare
hombit
force-pushed
the
light-curve-package
branch
from
150f64c to
797e8a6
Compare
There was a problem hiding this comment.
This looks good! I think the general implementation is solid, don't really have any critiques or suggestions, I did have one comment on the test suite, which I'd like to see fixed before approval. Generally, I'd also recommend adding something to the documentation on this, but were you thinking that TAPE eclipsing binary showcase would serve as a demo for using this analysis function? If that's the case I'm happy to let this move through without a tutorial for now.
| @@ -27,6 +27,7 @@ dependencies = [ | |||
| 'coverage', | |||
| "deprecated", | |||
| "ipykernel", # Support for Jupyter notebooks | |||
| "light-curve>=0.7.3,<0.8.0", | |||
There was a problem hiding this comment.
Out of curiosity, what's the pin to <0.8.0 motivated by, is this something that we would need to change to enable more recent versions of light-curve?
There was a problem hiding this comment.
I'm trying to follow semantic versioning conventions, so potentially I can brake backward compatibility in the 0.(x+1).0 version. Practically I don't brake it in a terrible or unsound way, but I still would like the version to be restricted.
| rows = {column: np.concatenate([object1[column], object2[column]]) for column in object1} | ||
|
|
||
| cmap = ColumnMapper(id_col="id", time_col="time", flux_col="flux", err_col="err", band_col="band") | ||
| ens = Ensemble().from_source_dict(rows, cmap) |
There was a problem hiding this comment.
In the test suite, try to make sure that any spun up ensemble objects use the dask_client defined in conftest.py, this avoids multiple clients being spun up during the test suite run
|
Thanks @dougbrn! I definitely should expand docstring and add a (small) tutorial |
hombit
force-pushed
the
light-curve-package
branch
from
b37d5de to
25698c6
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
No description provided.