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 get_observer_state #19

Merged
merged 15 commits into from
Jun 29, 2023
Merged

Add get_observer_state #19

merged 15 commits into from
Jun 29, 2023

Conversation

moeyensj
Copy link
Member

@moeyensj moeyensj commented Jun 8, 2023

Two main additions:

  • Added get_observer state with unit tests (we are able to get observer states to within ~10 meters or so, which is pretty good)
  • Added to_dataframe, from_dataframe definitions for Times that stores the time scale as part of the column name (this is important for round-tripping from a dataframe back to an instance).

Note that the line change counts are dominated by the addition of test files that each contain a few hundred data points.

The next PR will focus on making an actual Observers quivr table that will store these states.

@moeyensj moeyensj changed the title Add get_observer_state [WIP][DO NOT MERGE] Add get_observer_state Jun 8, 2023
@moeyensj
Copy link
Member Author

moeyensj commented Jun 9, 2023

I've got another commit in the works that will optimize the linear algebra in get_observer_state. Not that is super slow right now but it's kind of fun to make it faster.

@moeyensj
Copy link
Member Author

moeyensj commented Jun 9, 2023

Benchmarks at commit 8e2da8c:
Screenshot from 2023-06-09 08-48-04

Benchmarks at commit c2718ae:
Screenshot from 2023-06-09 08-49-39

Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

Intermediate comments, thanks for thoroughly commenting get_observer_state

return df

@classmethod
def from_dataframe(cls, df: pd.DataFrame) -> "Times":
Copy link
Contributor

Choose a reason for hiding this comment

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

It's useful to know about typing.Self for cases like this.

Suppose I subclass Times, making my own class TDBTimes(Times) or something. What is TDBTimes.from_dataframe's return type?

As you have written here, it's a Times - the superclass. Which is not quite right, but mypy and other type checkers will just trust you on that, which could lead to spurious type checker errors.

The way out of this is to say that from_dataframe returns type Self. That way, you're telling mypy that the return type is "whatever cls is". So it would all work.

typing.Self was added only in Python 3.11, but it's backported to be available via typing_extensions as well.

Now - I don't think it matters practically for this case, nobody is really going to subclass Times, but I notice whenever a constructor method like this doesn't return Self.

Comment on lines +10 to +11
cos_phi = Float64Column()
sin_phi = Float64Column()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a little note on what these physically represent? Don't go crazy writing a lot, you can just link to a wikipedia article or whatever if you want.

Comment on lines +27 to +31
code=OBSCODES["code"].values,
longitude=OBSCODES["Longitude"].values,
cos_phi=OBSCODES["cos"].values,
sin_phi=OBSCODES["sin"].values,
name=OBSCODES["Name"].values,
Copy link
Contributor

Choose a reason for hiding this comment

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

As of quivr v0.4.2, you don't need to use .values on pandas.Series objects anymore, you can pass them in directly to from_kwargs.

adam_core/observers/observers.py Outdated Show resolved Hide resolved

Currently only supports ground-based observers.

The Earth body-fixed frame used for calculations is the standard ITRF93, which takes into account:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything that you're aware of that you don't take into account? I don't have anything in mind, but it would be useful to spell out if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question, I got this from the SPICE documentation so I'm not sure what is missing. It should be a more robust frame than the standard IAU_EARTH that comes defined in DE440.

adam_core/observers/state.py Show resolved Hide resolved
adam_core/observers/observers.py Outdated Show resolved Hide resolved
adam_core/observers/state.py Outdated Show resolved Hide resolved
Comment on lines 84 to 85
r_obs = state.r
v_obs = state.v
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the really big else block, return early here, which makes it clear that you really will do no more work.

Comment on lines 107 to 109
unique_epochs_et = np.array(
[sp.str2et("JD {:.16f} TDB".format(i)) for i in unique_epochs_tdb]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this slow? This looks slow.

I don't think it ought to be necessary to call spicepy for this. I think it should be possible to do this conversion yourself, since it's really just a change of epoch from JD epoch up to J2000. It might even be a constant shift for all values?

This is only worth investigating if it is indeed slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's definitely slow. The conversion to ephemeris time in spiceypy requires the leapseconds kernel so I've just assumed it's doing something a bit more complex than what I can do outside of spiceypy, but TDB to ET should be nearly the same... Let me change it and see if the unit test tolerance changes at all.

Copy link
Contributor

@akoumjian akoumjian left a comment

Choose a reason for hiding this comment

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

Haven't had a check to review tests yet. Partial feedback.

adam_core/coordinates/times.py Show resolved Hide resolved
df_filtered = df.filter(regex=".*jd[12]_.*", axis=1)

# Extract time scale from column name
scale = df_filtered.columns[0].split("_")[-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to validate scale against an enum / set of some kind? Might catch problems earlier.

adam_core/observers/observers.py Outdated Show resolved Hide resolved
adam_core/observers/observers.py Show resolved Hide resolved
adam_core/observers/state.py Outdated Show resolved Hide resolved
adam_core/observers/state.py Outdated Show resolved Hide resolved
@moeyensj moeyensj merged commit e32137b into main Jun 29, 2023
1 check passed
@moeyensj moeyensj deleted the jm/observer-states branch June 29, 2023 23:19
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.

3 participants