add DR4 module into orbitize w/ OHP datafiles

[clarissardoo]

Added a new class to gaia.py for DR4, based on OHP conference slides parametrization.

New tutorial notebook (will be updated soon) into tutorials folder.

Edited priors, result, plot and system files to accommodate new addition to DR4.

[sblunt]

You've got a lot of logic for setting random seeds throughout the nb-- you only need to set the seed once at the beginning.

[clarissardoo]

done

[sblunt]

Maybe you're already talking to Vidya about this, but I think this bootstrap method should be in the code and not in the tutorial.

[clarissardoo]

done

[sblunt]

You can use orbitize.gaia.HGCALogProb._linear_pm_fit rather than reproducing the logic here

[clarissardoo]

done

[sblunt]

Don't love this; we should fix this so orbitize doesn't require an empty data table as input.

[clarissardoo]

done

[sblunt]

Use orbitize.sampler.examine_chains

[clarissardoo]

done

[sblunt]

don't need plt.show() in a notebook

[clarissardoo]

done

[sblunt]

use orbitize.results.print_results()

[clarissardoo]

done

[sblunt]

Why are M1 and plx so far away from the published values?

[clarissardoo]

done

[sblunt]

This feels a little clunky. Instead of copying the results object, perhaps we could add a kwarg to the orbit plot to plot the MAP orbit (I'd rather plot MAP instead of maxlike), and we can use that kwarg here?

[clarissardoo]

done

[sblunt]

This one reads more like an end-to-end test than a tutorial (which is fine)-- I think we should make it a .py script and move it to tests/end-to-end tests. Eventually we might remove it since the data will be in the EAS format, but let's keep it for now. It's a nice verification!

[clarissardoo]

left it as notebook for now, we can talk about this

[sblunt]

We should also add an input option for a user-specified method of combining the CCD scans, and also a method option of not combining them.

[clarissardoo]

done

[sblunt]

Once they are incorporated into the code, it would be great to add some unit tests for the bootstrap combination method and actual likelihood calculation. For the bootstrap combination, you can test that behavior remains the same over time (e.g. test that combining via bootstrap gives you the output you expect, which you can hard-code in), and for the likelihood, similar idea.

[sblunt]

We should take users through the process of downloading the data here. The xml file is ~1 MB, so I don't think we should distribute it with orbitize.

[sblunt]

The comment is a little confusing here-- you imply you're going to create the model, but then go to solving for the linear max likelihood solution for the astrometric params and setting the MCMC start positions. In general, one sentence of intro for each cell would be helpful throughout the nb!

[sblunt]

Hm, I don't like this. I think we should let the users pick the priors, and priors should not be set in this part of the code. I also don't think we should use Gaussian priors from the Gaia catalog as the defaults; I'd rather use uniform priors and let orbitize refit them.

[sblunt]

It would be nice to latex-ify this so it shows up nicely in the docs

[sblunt]

Make sure you define all the variables

[sblunt]

I think we should keep these variable names consistent with astrometric fitting in other parts of orbitize (e.g. the hipparcos module)-- pm_ra, pm_dec, alpha0, delta0

[sblunt]

No, don't expose "inject manually" option to user -- keep all the model setup stuff in system

[sblunt]

Keep this consistent with how we did the model setup in hipparcos.py-- do all the model setup stuff in system, don't inject stuff here

[sblunt]

I don't like this behavior; I'd rather the code fail if the user inputs an argument the code doesn't use

[sblunt]

Write out Args in docstring

[sblunt]

Both of these are imported at top; don't need to import them again

[sblunt]

I don't love the red stars as the default. Why don't you experiment with the colors a bit? I want it to look both recognizably "orbitize!" and recognizably "Clarissa"

[sblunt]

Hm. I'd rather fix the part of the code that is picking the RV + DR4 basis than add this in here

[sblunt]

Ok, overall it's looking great! Fantastic job putting this together; I know it was a ton of work. I left a lot of small comments, many about trying to use existing orbitize! functionality rather than reinventing the wheel, or following existing patterns or structures from other parts of the code.

Feel free to raise issues for things you don't want to address now. It would be good to get this code incorporated into DR4 sooner and make edits later.

[sblunt]

I don't like the catchall * input arg-- can you replace with specific args or kwargs?