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

Use cached_property and types #1718

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

aarchiba
Copy link
Contributor

Addresses part of #1708 and also demonstrates gradual typing towards #1709. Unsure whether this resolves the first.

@aarchiba
Copy link
Contributor Author

aarchiba commented Feb 10, 2024

I am hoping that the type annotations improve the readthedocs build. ETA: looks like they do, at least to provide a list of attributes in a class, and to provide clickable types for arguments and return values.

Copy link

codecov bot commented Feb 10, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (fa790eb) 68.96% compared to head (6fda954) 69.09%.
Report is 15 commits behind head on master.

Files Patch % Lines
src/pint/observatory/topo_obs.py 96.07% 1 Missing and 1 partial ⚠️
src/pint/observatory/__init__.py 98.07% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1718      +/-   ##
==========================================
+ Coverage   68.96%   69.09%   +0.13%     
==========================================
  Files         105      105              
  Lines       24696    24729      +33     
  Branches     4409     4409              
==========================================
+ Hits        17031    17087      +56     
+ Misses       6560     6534      -26     
- Partials     1105     1108       +3     

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

@aarchiba aarchiba added enhancement awaiting review This PR needs someone to review it so it can be merged minor A minor PR that doesn't need a lot of thought labels Feb 18, 2024
@@ -428,17 +468,17 @@ def get_TDBs(self, t, method="default", ephem=None, options=None):
else:
raise ValueError(f"Unknown method '{method}'.")

def _get_TDB_default(self, t, ephem):
def _get_TDB_default(self, t: astropy.time.Time, ephem):
Copy link
Contributor

Choose a reason for hiding this comment

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

does ephem not get a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add it because I wasn't sure what the type was. String I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it's a string

return t.tdb

def _get_TDB_ephem(self, t, ephem):
def _get_TDB_ephem(self, t: astropy.time.Time, ephem):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

and later

@@ -277,11 +306,11 @@ def get_dict(self):
return {self.name: output}

def get_json(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to apply incremental typing just to those functions I touched. But if we're going all in on type annotations then I can be more aggressive.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sure. either way.

return json.dumps(self.get_dict())

def separation(self, other, method="cartesian"):
"""Return separation between two TopoObs objects
def separation(self, other: "TopoObs", method: str = "cartesian"):
Copy link
Contributor

Choose a reason for hiding this comment

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

return type?

for clock in self._clock:
t = min(t, clock.last_correction_mjd())
return t

def _get_TDB_ephem(self, t, ephem):
def _get_TDB_ephem(self, t: Time, ephem) -> Time:
Copy link
Contributor

Choose a reason for hiding this comment

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

type of ephem?

@@ -389,8 +416,8 @@ def _get_TDB_ephem(self, t, ephem):
# Topocenter to Geocenter
# Since earth velocity is not going to change a lot in 3ms. The
# differences between TT and TDB can be ignored.
earth_pv = objPosVel_wrt_SSB("earth", t.tdb, ephem)
obs_geocenter_pv = gcrs_posvel_from_itrf(
earth_pv: PosVel = objPosVel_wrt_SSB("earth", t.tdb, ephem)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the advantage to typing of this sort? Is this to prevent any changes to the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells the type checker to signal a problem if the function does not actually return a PosVel, or if the surrounding code uses the value in a way incompatible with a PosVel. More, it informs the reader what type this has, in case they don't know off the top of their head what type to expect. In this case it seemed useful because I wasn't ready to go digging about and annotate those two functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged enhancement minor A minor PR that doesn't need a lot of thought
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants