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.

Sign up for GitHub

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

Jump to bottom

tickets/SP-1600: updates to pass ruff checks #429

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

tickets/SP-1600: updates to pass ruff checks #429

wants to merge 5 commits into from
+148 −97

Conversation

humnaawan
Copy link

No description provided.

@humnaawan humnaawan marked this pull request as draft October 31, 2024 13:11
@humnaawan humnaawan marked this pull request as ready for review October 31, 2024 13:28
@rhiannonlynne
Copy link
Member

Sorry. - I should have provided more context around ephem ... I was kind of curious to see what you'd do about this particular issue, actually.
We dropped ephem (we had been using it for some almanac-style calculations of sunset and planet positions), and do not want to put it back in.
This particular metric (the star counts) doesn't run in standard use, so it's not worth us adding another dependency to support this particular conversion between RA/Dec and galactic coordinates.
It's also not clear why ephem would have to be used to do this conversion (and I don't know why there are three different methods to convert between ra/dec and galactic l/b here).
In short, this is a community contribution that has bit-rotted over time.
That's the additional context ... which leaves a couple of options:

  • add ephem as a dependency, keep using the ephem ra-dec to gal l-b conversion
  • check the difference between other conversions and the conversion you get from ephem, and swap to one of the other conversions that is available using some other method (astropy would be my preferred option .. SkyCoord converts between ra/dec and l/b in a widely-used way) -- dropping the ephem pieces of the code
  • just use one of the other conversions that's written into this piece of code .. my take on what is provided here is that probably one of the other methods was used first, then the contributor found that ephem provided a more accurate conversion and switched. However, I did check the conversion using the analytic method here (that doesn't use ephem) against astropy's conversion, and it's at most a few degrees incorrect. Which probably doesn't make that much of a difference in the metric output .. especially for a metric we're not actually running. So we could drop the ephem conversion and just use the analytic method and say it's close enough.
  • just drop the entire piece of code. I didn't do this in the first place, because having an analytic way to calculate stellar density and distribution over distance at any point in the sky seemed useful .. but tbh, I don't know how accurate it is, and other metrics don't use it. And probably wouldn't use it, without further checking and validation. So I suspect, in the end, this is likely the best option.

rhiannonlynne
rhiannonlynne requested changes Nov 2, 2024
View reviewed changes
Copy link
Member

@rhiannonlynne rhiannonlynne left a comment

Choose a reason for hiding this comment

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

I don't want to add ephem back into the dependencies, so we should probably either deprecate all of the star_counts code that needs it, or move to a different ra/dec to galactic l/b conversion that doesn't depend on ephem.

rubin_sim/maf/metrics/transient_metrics.py

Parameters
----------
data_slice : numpy.array
Numpy structured array containing the data related to the visits provided by the slicer.
Numpy structured array containing the data related to the
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong level of indent I think -- only four spaces indent (and not tabs) for parameter descriptions.

rubin_sim/maf/metrics/visit_groups_metric.py

Parameters
----------
min_gap : float, optional
Minimum time to consider something part of a pair (minutes). Default 15.
Minimum time to consider something part of a pair (minutes).
Copy link
Member

Choose a reason for hiding this comment

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

Wrong level of indent for a parameter description.

rubin_sim/maf/metrics/visit_groups_metric.py
@@ -65,7 +69,8 @@ def run(self, data_slice, slice_point=None):


class VisitGroupsMetric(BaseMetric):
"""Count the number of visits per night within delta_t_min and delta_t_max."""
"""Count the number of visits per night within delta_t_min
and delta_t_max."""
Copy link
Member

Choose a reason for hiding this comment

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

This matches ruff, but since you're here editing ...
the docstring after the "init" below with the parameters should come up to this section of documentation as Parameters and reformat into numpydoc style.

Copy link
Author

@humnaawan humnaawan Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would it be okay to leave this update for the issue focused on aligning with with developer guide?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, ok.

rubin_sim/maf/metrics/visit_groups_metric.py
"""Reduce to max number of total visits on all nights with more than minNVisits,
within any 'window' (default=30 nights)."""
"""Reduce to max number of total visits on all nights with more than
minNVisits, within any 'window' (default=30 nights)."""
Copy link
Member

Choose a reason for hiding this comment

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

The """ should go on a new line. (rubin docstyle )

Copy link
Author

Choose a reason for hiding this comment

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

would it be okay to leave this update for the issue focused on aligning with with developer guide?

Copy link
Member

Choose a reason for hiding this comment

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

ok.

@humnaawan
Copy link
Author

regarding whether to keep ephem or not, would it helpful to contact the contributor (seems like its Mike Lund)? i wonder if they are running the metric, even if its not run in the regular use.

@rhiannonlynne
Copy link
Member

regarding whether to keep ephem or not, would it helpful to contact the contributor (seems like its Mike Lund)? i wonder if they are running the metric, even if its not run in the regular use.

Eek, I thought I'd replied to this. Yes, it's very reasonable to contact Mike Lund.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhiannonlynne rhiannonlynne rhiannonlynne requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@humnaawan @rhiannonlynne