John Parejko added a comment - 13/Jan/21 1:20 AM Please file a ticket to write tests for the gen3 reference loader. I also noted on the PR about getting a ticket to fix our broken use of EPOCH in that metadata.

John Parejko added a comment - 13/Jan/21 1:25 AM Thank you for doing this work. I'm not happy about the testing situation, but that's not really your fault. I guess this gives us a reason to accelerate implementation of DM-27013 ?

Lauren MacArthur added a comment - 13/Jan/21 3:12 AM Thanks for the review, John.  As to the whole "EPOCH" and what it does/should refer to calamity, I need some help from you as I thought I was "following your lead" (where, really I mean I was taking examples from jointcal , more below) in the expectations of what we are passing to the proper motion correction implementation as a variable called "epoch".  It seems to me that many [most/all?] places that we pass around something with the variable name epoch , we do indeed mean for it to be MDJ.  To start, the Gaia refcats have entries like: In [ 52 ]: gaiaRefCatDf[ 'epoch' ] Out[ 52 ]: id 3836068354164430080 57205.874409 3836070896784313984 57205.874409 ... which, at first glance, look like MJD to me...but looking at the documentation (warning before clicking...big file) here EPOCH : Gaia-centric epoch TCB(Gaia) (double, Time[Barycentric JD in TCB - 2455197.5 (day)]) whatever that implies for code in applyProperMotionsImpl noted below... On the other hand, ps1 has: In [ 53 ]: ps1RefCatDf[ 'epoch' ] Out[ 53 ]: id 110681501036355632 1320442112 110671501040283925 1397608704 ... which is a bit mysterious... Regardless, looking at applyProperMotionsImpl , the following seems to imply units of MJD for the "epoch" of both the reference and source catalogs: catEpoch = astropy.time.Time(catalog[ "epoch" ], scale = "tai" , format = "mjd" ) # Use `epoch.tai` to make sure the time difference is in TAI timeDiffsYears = (epoch.tai - catEpoch).to(astropy.units.yr).value so these ( catEpoch and epoch ) would need to be in consistent "units". Also, in jointcal , it again seems to expect and use MDJ as what is represented by the term "epoch" def _compute_proper_motion_epoch( self , ccdImageList): """Return the proper motion correction epoch of the provided images. Parameters ---------- ccdImageList : `list` [`lsst.jointcal.CcdImage`] The images to compute the appropriate epoch for. Returns ------- epoch : `astropy.time.Time` The date to use for proper motion corrections. """ mjds = [ccdImage.getMjd() for ccdImage in ccdImageList] return astropy.time.Time(np.mean(mjds), format = 'mjd' , scale = "tai" ) I also see your (and TimJ's) point about "standards" and expectations for such terms from the community, but we seem to be in a bit of a mess here (and I also appreciate your point that daf_base adopts "EPOCH is Julian epoch year"), and I'm quite sure I'm not the one to make any calls on changing things (I'm well-enough placed to make a best attempt at keeping all uses within our codebase consistent, but have no horse in the race as to how they should be defined and used). I could easily make a ticket to say we should "standardize the use of the term EPOCH, whether it is being used as a variable name for use in computations or metadata header that gets stored", but if you are advocating (quoting from the PR): ...to remove that epoch keyword and replace it with either EQUINOX (following the FITS standard) which would then be a Julian year, or MJD if we want to keep it as MJD. Julian year makes more sense to me as it is the value you would directly use in proper motion calculations, with values given in mas/year. I might ask you to file it with your desiderata and their reasonings (I don't feel comfortable suggesting the above since I'm not convinced/qualified to judge the fidelity of that claim...man, I can't even say for sure what is in the Gaia catalogs as "epoch", and that is a crucial piece of understanding required if we're going to get this right!)

Lauren MacArthur added a comment - 13/Jan/21 3:22 AM Ticket for Gen3 reference loader tests is DM-28321 .

John Parejko added a comment - 13/Jan/21 8:13 PM Ok, I did the deep dive on uses of "epoch" in the stack. Looks like the only real culprits are the one we found here, and some in sdm_schemas/dax_apdb. See DM-28329 for (many) details.