zpenoyre / astromet.py

building and fitting one & two body astrometric tracks
https://astrometpy.readthedocs.io/en/latest/
GNU General Public License v3.0
18 stars 9 forks source link

astromet.parms() default parameters are risky and verbose to set #8

Open emilyhunt opened 2 years ago

emilyhunt commented 2 years ago

Hey, firstly I have to say thank you for making this lovely piece of software, it is immeasurably useful for anything to do with unresolved binary astrometry with Gaia! I have a few ideas & things I've noticed about the module though and if that's ok I'll submit a couple of GitHub issues for the next few days while I'm using the module & as ideas appear.

So with astromet.params(), I noticed a couple of things:

Instead, I'd suggest something like this, which would allow the user to specify as many parameters that they need at initialisation of the object, and would also have some basic error checking (since ra, dec etc. would be mandatory to specify for every track, which I think makes sense anyway!)

class params():
    def __init__(self, ra, dec, pmrac, pmdec, parallax, drac=0.0, ddec=0.0, 
                 period=1.0, a=0.0, e=0.1, q=0.0, l=0.0, vtheta=0.0, vphi=0.0, 
                 vomega=0.0, tperi=0.0, epoch=2016.0):
        # Mandatory astrometric parameters
        self.ra = ra  # degree
        self.dec = dec  # degree
        self.pmrac = pmrac  # mas/year
        self.pmdec = pmdec  # mas/year
        self.parallax = parallax  # mas

        # Optional astrometry
        self.drac = drac  # mas
        self.ddec = ddec  # mas

        # Optional binary parameters
        self.period = period  # year
        self.a = a  # AU
        self.e = e
        self.q = q
        self.l = l  # assumed < 1 (though may not matter)
        self.vtheta = vtheta
        self.vphi = vphi
        self.vomega = vomega
        self.tperi = tperi  # jyear

        # Below are assumed to be derived from other params
        # (I.e. not(!) specified by user)
        self.totalmass = -1  # solar mass
        self.Delta = -1

        # the epoch determines when RA and Dec (and other astrometry)
        # are centred - for dr3 it's 2016.0, dr2 2015.5, dr1 2015.0
        self.epoch = epoch

So e.g. you could now initialise a basic star with no binary with

params = astromet.params(ra=10, dec=0, pmrac=3, pmdec=-2, parallax=2.4)

which I think is a lot neater. Python would raise an error if any of the five fundamental parameters aren't specified. It would also be possible to make it so the user has to specify all binary parameters or else an error is raised (e.g. by folding them all into just *args (see this link for how) and checking that args contains all required binary parameters if any are specified at all.)

Thanks and I hope this helps!

emilyhunt commented 2 years ago

As an addendum to this, the binary viewing angles seem to be initialised in degrees currently (no unit is in the comments) but it looks like they should be in radians instead from the rest of the code, so the current defaults for viewing angle would be extra odd:

self.vtheta = 45
self.vphi = 45
self.vomega = 0

May also be worth adding in comments what exactly these parameters are (or using more explicit variable names), it's especially difficult to work out what vomega is & does.

zpenoyre commented 2 years ago

Thanks! This is a great suggestion that I plan to incorporate in the next few weeks (except the viewing angles, fixed those already) - I have a slightly different plan for the binary parameters, as we've just added blended and lensed sources too, so I want to split up and simplify the whole process. I really appreciate the feedback, let me know if you ever have more ideas/requests!