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

Add method for creating lightcurve for object #464

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Oct 7, 2024

This fixes #462 by adding a lightcurve_for method to PhotometryData.

A couple items:

  1. A better API might be to accept a single required argument, maybe called "target", that can be either a star_id, a coordinate or a name. If that is better, then we might need to drop the name option, because star_id is a string so I'm not sure how to distinguish between star_id and name.
  2. The term "flux" is a little unfortunate, but we can't change that...it is a lightkiurve thing.

@mwcraig mwcraig added this to the 2.0.0-beta milestone Oct 7, 2024
@mwcraig mwcraig requested a review from JuanCab October 7, 2024 21:38
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.53%. Comparing base (31f01ab) to head (1c89681).
Report is 79 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
+ Coverage   80.30%   80.53%   +0.23%     
==========================================
  Files          30       31       +1     
  Lines        3945     3987      +42     
==========================================
+ Hits         3168     3211      +43     
+ Misses        777      776       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JuanCab
JuanCab previously approved these changes Oct 8, 2024
Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable overall. The tests looked reasonable as well (I mean, you are just checking the right number of rows are in the final light curve).

@@ -193,5 +194,7 @@ filterwarnings = [
# papermill is using deprecated jupyter paths
'ignore:Jupyter is migrating its paths to use standard platformdirs:DeprecationWarning',
# papermill is also using a deprecated method of getting current time
'ignore:.*is deprecated and scheduled for removal.*:DeprecationWarning'
'ignore:.*is deprecated and scheduled for removal.*:DeprecationWarning',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how happy I am with ignoring this warning, given the warning includes "scheduled for removal". :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree, but also can't control 😬

@mwcraig
Copy link
Contributor Author

mwcraig commented Oct 8, 2024

A better API might be to accept a single required argument, maybe called "target", that can be either a star_id, a coordinate or a name.

@JuanCab what do you think about this 👆

@JuanCab
Copy link
Contributor

JuanCab commented Oct 9, 2024

A better API might be to accept a single required argument, maybe called "target", that can be either a star_id, a coordinate or a name.

@JuanCab what do you think about this 👆

Yeah, I don't know if the technical term for this is API (we aren't connecting different programs together via some server/client interface), BUT I do think it make sense to have a single required argument, especially if the only difference is the data type.

@mwcraig
Copy link
Contributor Author

mwcraig commented Oct 9, 2024

Merging, since the new signature matches the proposal above (darn you for learning what things like API actually mean 😉 ) and test coverage is 100% and tests are passing.

@mwcraig mwcraig merged commit cb1dcda into feder-observatory:main Oct 9, 2024
18 checks passed
@mwcraig mwcraig deleted the to_lightcurve-and-beyond branch October 9, 2024 14:54
@mwcraig mwcraig restored the to_lightcurve-and-beyond branch October 9, 2024 14:54
@mwcraig mwcraig deleted the to_lightcurve-and-beyond branch October 9, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a conveniences method for PhotometryData
2 participants