BrownDwarf / gollum

A microservice for programmatic access to precomputed synthetic spectral model grids in astronomy
https://gollum-astro.readthedocs.io/
MIT License
20 stars 5 forks source link

Rotational fitting #108

Closed DanielAndreasen closed 4 months ago

DanielAndreasen commented 4 months ago

This tutorial could use some polishing: https://gollum-astro.readthedocs.io/en/latest/tutorials/best_fit_for_fixed_template.html

  1. Some weird formatting in the first paragraph here, where you want to write vsin(i) (closing #107)
  2. It feels like it assumes the user already have muler installed. Just add a small paragraph saying it can be installed with pip install muler, so the user don't have to find this information.
  3. In code block 3, where you define the path to the spectrum, I suggest using a more descriptive path. Maybe something like ~/Downloads/Goldilocks_20210517T054403_v1.0_0060.spectra.fits.
  4. Not at all necessary, but the data_clean function can be writte quite elegantly (in my opinion) like below (I like to use type annotations), since all the methods used on the data, return the same type. But I do recognize that for some users this might be confusing.
  5. Code block 8 and other places. Semi-colons are not illegal at the end of a line in python, but they should be.
  6. Code block 9: I believe metallicity should be replaced with Z.
  7. Code block 15: the chi2 calculations gives me a bunch of inf. Should it be: chi2_loss = np.sum(0.5 * residual.flux**2) / np.sum(residual.uncertainty.array**2)? If I change it to that, the code works for me and give the same results as written in the tutorial.
def data_clean(data: HPFSpectrumList) -> HPFSpectrum:
    """Clean the HPF data with standard post-processing techniques"""
    return (
        data.sky_subtract(method="vector")
        .deblaze()
        .trim_edges((4, 2942))
        .normalize()
        .stitch()
    )

Review link: https://github.com/openjournals/joss-reviews/issues/6601

Sujay-Shankar commented 4 months ago

Thanks for the feedback on the docs! Admittedly they're quite old and have flown under my radar until now, but all your points make sense. I'll address as many as possible and let you know how it goes.

DanielAndreasen commented 4 months ago

Sounds good. Let me know if I can help somehow

Sujay-Shankar commented 4 months ago

Ok, I think I've solved all the issues here:

  1. Math formatting inside italics doesn't work, so I got rid of the italics.
  2. I downloaded the data into gollum to both shorten the path and take the burden off the user for wrangling with path stuff
  3. I removed the data clean function, it was a one-off, there wasn't really a need for it to be a function in the first place.
  4. I removed all the semi-colons and put in our favorite plt.show()
  5. metallicity was replaced with Z
  6. The formula we used previously as far as I know doesn't look like the chi-square formula, and the description was sum of squares of residuals, which also isn't chi-square, so I switched the whole thing to using RSS (results are the same, and it's much simpler)

Aside from those, just generally spruced up the page with modern stuff like f-strings, concise numpy stuff, and cleaner figures.