use jplephem and python instead of SPICE toolkit

[aelanman]

The dependency on spiceypy has been weighing this down a bit. The jplephem package is already required for handling ephemerides in astropy, but wasn't fully equipped to parse PCK kernels required for the lunar frames. With some AI help, we now have a PCK reader and have replaced all spiceypy dependencies. This also obviates all station_id tracking, since we don't need to make temporary ephemeris files for each lunar surface point in use.

I'm going to continue verifying the changes, but tests are passing.

[codecov]

Codecov Report

❌ Patch coverage is 96.55172% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.48%. Comparing base (7c0e88d) to head (5fcd27a).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lunarsky/spice_utils.py	95.16%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   86.73%   87.48%   +0.74%     
==========================================
  Files           8        8              
  Lines         618      639      +21     
==========================================
+ Hits          536      559      +23     
+ Misses         82       80       -2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bhazelton]

This seems like a very promising direction! I think it'd be good to test it in the context of pyuvsim to see if it changes anything unexpectedly. Maybe @wps2n could help with that? @jpober you should look at this too.

[jpober]

@burdorfmitchell this test might be up your alley once your prelim is behind you

[aelanman]

@burdorfmitchell -- If you get a chance to try this out, I'd really appreciate it! It should be the same interface as pyuvsim currently expects.

[burdorfmitchell]

Sorry for taking a bit to get to testing this, I'll take a look at it later tonight. I assume I should be running some lunar pyuvsim simulations and examining performance and output consistency between this pr and main.

[aelanman]

Hi @burdorfmitchell ; thanks for testing this out! Yes, if you have a reference lunar simulation and can compare the results with this branch to the results before, that would be very helpful.

[bhazelton]

@aelanman can you please check that the pyuvdata, pyradiosky and pyuvsim tests pass with this branch installed?

[aelanman]

@bhazelton I've tested pyuvsim/data and pyradiosky with this branch, and can confirm all tests are passing. However, pyradiosky and pyuvdata have hooks in place to catch "SpiceUNKNOWNFRAME" exceptions, which should no longer be necessary since spiceypy will be removed. This may break in the GH workflow because lunarsky won't install spiceypy as a dependency.

I don't remember exactly why I switched away from using astropy's download_files_in_parallel... I think maybe I just wanted the kernel files in the package directory for some reason? I'll switch to using astropy for caching.

[aelanman]

@bhazelton Is this just waiting on @burdorfmitchell 's tests? Since I've confirmed this doesn't break the tests in the rest of the RASG packages, this should be ready to merge. I can open PRs to remove the spiceypy dependency in the other packages (tests no longer need it).

[bhazelton]

Yes, I think it's just waiting on @burdorfmitchell 's tests. Prepping the other repos for when this is merged seems like a good idea.

[burdorfmitchell]

Sorry for the delay. I have tested the changes locally and on the github cli and find that pyuvsim's lunar reference simulation changes significantly using bland instead of main. Below is a list of some net statistical differences between the visibilities of the reference simulation with old lunarsky and this pr:

mean of abs of diff of visibilities 'mean(abs(old_data_arr - new_data_arr))': 3.077895608032528e-05
mean of diff of abs of visibilities 'mean(abs(old_data_arr) - abs(new_data_arr))': -1.230521653059482e-07
max_absolute_diff: 0.0006231183852107216
max_relative_diff: 0.0012467779104614499

I tested this locally on Brown's HPC cluster and also on the github CI (here) (ignore the failing external tests and other test failures are due to downloads failing).

While this is comparing against a stored reference simulation output produced with an older version of lunarsky, I also ran the reference simulation with the latest lunarsky on main and got exact agreement with historical output.

If a change in simulation output is expected, then I am fine updating the reference simulation on the Brown Digital Repository to match that.

Besides that, everything except for one thing looks like it should be passing with your pyuvsim pull request (after it is rebased on top of main again). There may be a need to increment the lunarsky version in some ci files as well but that can be belated.

On this line, I believe you open a file and never close it, which creates a warning which makes the warning test fail on pyuvsim.

[aelanman]

Hi @burdorfmitchell , thank you for running those tests, and spotting the unclosed file handle.

I just double-checked that the transformations to and from lunar frames, and the sidereal time calculation, are consistent between main and bland. What version of lunarsky was used to make the reference sims? I can try comparing with that instead. It's not clear to me why the visibilities should change if these are consistent.

[bhazelton]

Hi @burdorfmitchell , thank you for running those tests, and spotting the unclosed file handle.

I just double-checked that the transformations to and from lunar frames, and the sidereal time calculation, are consistent between main and bland. What version of lunarsky was used to make the reference sims? I can try comparing with that instead. It's not clear to me why the visibilities should change if these are consistent.

It sounds like @burdorfmitchell is saying that if he runs the reference sims on main on pyuvsim they match the saved outputs, so I don't think we need to go back to compare with whenever the saved outputs come from, we just need to compare against main.

I suspect that while main and bland match most of the time, there are some edge cases that are being triggered in this reference sim. We probably need to go through it in detail to understand where the differences are coming in. There are some docs on running the reference sims locally, maybe we should do that so we can compare uvws and SkyModel calculated values (like alt_az and pos_lmn and maybe above_horizon) to see which things aren't matching on main and bland. Those seem like the most likely things to be different, but maybe it's something else.

[aelanman]

@bhazelton @burdorfmitchell I think I've found the issue. This branch also updates all the transformations to use the TDB (barycentric dynamical time) instead of the TT (terrestrial time). This was a mistake before; the ephemeris time used by spice and by jplephem should be TDB, not terrestrial time, since the ephemeris kernels are tabulated in TDB. The difference between the two can be hundreds of milliseconds, during which time the moon travels about 20 meters in the barycentric frame. This would then affect any baseline transformations involving ICRS.

One way to check if this is the problem is to temporarily change tdb to tt in the following lines of lunarsky and see if the result matches the reference sim results from before:

lunarsky/topo.py:132:    ets = (obstime.tdb - _J2000).sec
lunarsky/spice_utils.py:320:    ets = (obstimes.tdb - Time("J2000")).sec
lunarsky/time.py:73:        et = np.atleast_1d((self.tdb - Time("J2000")).sec)
lunarsky/mcmf.py:69:    ets = np.atleast_1d((obstime.tdb - _J2000).sec)

If that fixes it, then it means the new ref sim results are more correct and should supersede the original.

[burdorfmitchell]

That does change the output a bit but I still see similar disagreement. Temporarily changing back to tt:
(I keep in the two Time("J2000", scale="tt") lines because I am also wondering if those should have been changed to tdb not that it should resolve this discrepancy)

$ grep -nrwI tt | grep -v test
mcmf.py:22:_J2000 = Time("J2000", scale="tt")
mcmf.py:69:    ets = np.atleast_1d((obstime.tt - _J2000).sec)
spice_utils.py:320:    ets = (obstimes.tt - Time("J2000")).sec
time.py:73:        et = np.atleast_1d((self.tt - Time("J2000")).sec)
topo.py:35:_J2000 = Time("J2000", scale="tt")
topo.py:132:    ets = (obstime.tt - _J2000).sec

when I run the reference simulation I get this:

mean of abs of diff of visibilities 'mean(abs(old_data_arr - new_data_arr))': 3.9855599899696213e-05
mean of diff of abs of visibilities 'mean(abs(old_data_arr) - abs(new_data_arr))': -1.228933264247369e-07
max_absolute_diff: 0.0006532461534090945
max_relative_diff: 0.0012610404089415095

[aelanman]

Hi @burdorfmitchell -- where are those deviations appearing? Do they vary in a noticeable way with frequency, baseline length or angle?

[aelanman]

@burdorfmitchell Could you send me the visibility files of both the reference sims and your rerun with this branch and tdb changed to tt?