DRAFT: DynamicSpec #35
Conversation
emolter
left a comment
emolter
left a comment
There was a problem hiding this comment.
Hi Sarah, just general feedback for now. I do think some re-structuring of various things would be beneficial for the legibility and flexibility of your code - see specific comments. Let me know if you'd like to Zoom about this sometime.
| centered_list = list(range(-half, half + 1,2)) | ||
| return centered_list | ||
|
|
||
| ############################################################################### |
There was a problem hiding this comment.
please remove extraneous comment lines
There was a problem hiding this comment.
I will for the final version, but while I'm still developing I find these very helpful to break up reading the code. It's easier for me to understand and read if there are clear delineations between different sections.
|
|
||
| ############################################################################### | ||
|
|
||
| def __init__(self, spec, rest_f, wl = None, freq = None): # stuff that is run when an object is defined |
There was a problem hiding this comment.
This is a stylistic thing, but I am in the habit of minimizing comments like these. This YouTube video explains the philosophy behind this relatively well (the concepts are clear even though I don't know the programming language used in the examples)
There was a problem hiding this comment.
This is a habit, mainly because I'm not the best at reading other people's code. I'll start moving comments like these to the doc string instead.
| showplots - indicator of whether to plot or not, string - 'y' or 'Y' will plot the fit results | ||
|
|
||
| initial_fit_guess (optional) - path to a saved LMFIT result to use as the initial guess. This can be beneficial if some spectra are very noisy. It is recommended that you use a pixel with as small a Doppler shift as possible to make your initial guess. | ||
| RMS (optional) - numerical value of the data's RMS, float |
There was a problem hiding this comment.
Functions that return things should also have a Returns section of the docstring.
Generally, for the docstrings, please do your best to make the format identical to the ones in the rest of the code base. Those are based on the Numpy style guide.
|
|
||
| return v, wind_err | ||
|
|
||
| model = {} |
There was a problem hiding this comment.
please use a more descriptive variable name. Something like, "lmfit_model_dict"
| ''' | ||
| Input string instantiates object using numpy arrays of the x-axis and the spectral intensity axis. | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
docstrings look much better! When I clone this to add dependencies to pyproject.toml, I'll also do my best to fix any residual formatting issues that are causing the readthedocs build to fail
| ---------- | ||
| spec : numpy array | ||
| The intensity/vertical axis of the spectrum | ||
| rest_f : float |
There was a problem hiding this comment.
I don't think the rest frequency needs to be in the init of Spectrum; for example, what if you wanted to fit two molecular lines in the same spectrum, or you wanted to test the effect of perturbing the rest frequency a little bit? This could instead be an input parameter to fit_profile.
| The intensity/vertical axis of the spectrum | ||
| rest_f : float | ||
| The rest frequency of molecular transition being examined | ||
| x_space : string |
There was a problem hiding this comment.
same with this one. I don't think we need to be opinionated about this until the function where lmfit requests it. For example, what if I wanted to try both? I wouldn't want to have to instantiate a new object
| self.wl = wl | ||
| self.freq = freq | ||
|
|
||
| if self.wl is None and self.freq is None: | ||
| raise ValueError('You did not provide an input for the x-axis. Please include a frequency (freq) or wavelength (wl) array.') | ||
|
|
||
| if self.wl is None: | ||
| self.wl = c/self.freq | ||
| elif self.freq is None: | ||
| self.freq = c/self.wl |
There was a problem hiding this comment.
| self.wl = wl | |
| self.freq = freq | |
| if self.wl is None and self.freq is None: | |
| raise ValueError('You did not provide an input for the x-axis. Please include a frequency (freq) or wavelength (wl) array.') | |
| if self.wl is None: | |
| self.wl = c/self.freq | |
| elif self.freq is None: | |
| self.freq = c/self.wl | |
| if wl is None and freq is None: | |
| raise ValueError('You did not provide an input for the x-axis. Please include a frequency (freq) or wavelength (wl) array.') | |
| if wl is None: | |
| self.wl = c/freq | |
| self.freq = freq | |
| elif freq is None: | |
| freq = c/wl | |
| self.wl = wl |
| print('Numerical results will be saved in the '+outfile+'.pickle file') | ||
| results = {'x':[],'y':[],'center':[],'std':[],'chi':[],'redchi':[]} | ||
|
|
||
| for xpix in range (0,self.xpixmax,1): |
There was a problem hiding this comment.
why does this happen over self.xpixmax and self.ypixmax, when you have already specified the input data? shouldn't it just be applied to all the data that were requested by this function itself?
| The platescale of the data. | ||
| body : string | ||
| The name of the planetary object you are studying. | ||
| cont_label : string |
There was a problem hiding this comment.
probably ok to set a default here
| ---------- | ||
| picklefile : string | ||
| The path to the pickle file of calculated doppler winds and errors. | ||
| platescale : float |
There was a problem hiding this comment.
is this not retrievable from the header?
|
|
||
| ############################################################################### | ||
|
|
||
| def plot_data(self, picklefile, platescale, body, cont_label, title = None, date = None, location = None, spatial = None, variable = None, cmap = None, limits = None, contours = None, hatch = None, savefig = None): |
There was a problem hiding this comment.
hard to evaluate this without seeing the results
|
|
||
| ############################################################################### | ||
|
|
||
| def wind_calc(self, picklefile, restfreq, outfile): |
There was a problem hiding this comment.
this function currently does not use self.anything; might be an indication it should exist outside of the Cube object?
|
Nice job Sarah. This is much easier to read code than before. Still quite a few things that could be cleaned up, but great job overall. May I request that you include a Jupyter notebook example with the next draft? Confusingly, this should not go inside the notebooks/ subfolder, but instead inside docs/ |
2 participants
Code that takes input FITS files and extracts the winds through the Doppler shift of emission spectra. Functionality includes extraction of a single spectrum or an entire image cube, deconvolution of the beam, and the extraction of a zonal wind profile.