Spectral Winds Pylanetary PR#67
Conversation
|
Neptune_ALMA_winds_workflow.ipynb |
|
Neptune_ALMA_pylanetary_refactor_test.py |
|
I haven't looked at this in detail just yet, but a few immediate things pop out:
|
emolter
left a comment
emolter
left a comment
There was a problem hiding this comment.
Thanks very much for this submission Rick! I only reviewed core.py through the end of the Spectrum class. Overall it looks like a really useful addition to the package, but I think there's some cleanup to do here to make things more flexible, clearer, and better documented.
| # ========================= | ||
| # Dataclasses for Spectrum() | ||
| # ========================= |
There was a problem hiding this comment.
Here and elsewhere the inline comments are not needed. If you think there's any reason to chunk this into different logical pieces, then it would be good to do so by putting them in separate submodules, e.g. pylanetary.spectral.plotting.
But this is only like 800 lines which I'd say is not so large it's necessary to do so.
There was a problem hiding this comment.
Do you mean you want me to delete the #======= section comments or the inline comments like x=y+2 # comment, or both?
There was a problem hiding this comment.
I think moving the diagnostic plotting and such to a different module makes sense. Did you want to huddle sometime and chat about that structure or can you elaborate more here on what you'd like.
| class SpectrumData: | ||
| """ | ||
| Container for a single spectrum input data. | ||
| """ |
There was a problem hiding this comment.
For the dataclasses could you please add Attributes sections to the docs?
| from matplotlib import pyplot as plt | ||
| import pickle | ||
|
|
||
|
|
There was a problem hiding this comment.
a __all__ section for all the stuff you'd consider "public" would be helpful here. Among other things it allows the readthedocs build to pick things up.
| # ========================= | ||
| # Model dictionary | ||
| # ========================= | ||
| model = {'Gaussian': {'ref': 'gauss', 'lmfit_func': GaussianModel(), 'label': 'Gaussian Fit', 'color': 'orange'}, |
There was a problem hiding this comment.
Can you give this a more descriptive name, and also make it all-caps?
| 'Moffat': {'ref': 'mof', 'lmfit_func': MoffatModel(), 'label': 'Moffat Fit', 'color': 'brown'} | ||
| } | ||
|
|
||
| # TODO This could be a separate constants.py file that is imported and modified outside of the module or a config file. |
There was a problem hiding this comment.
just commenting so we remember to open an issue for this (and any other TODOs here)
|
|
||
| self._check_signal_rms(input_data=self.filtered_snr_data) | ||
|
|
||
| if self.signal_flag: |
There was a problem hiding this comment.
what is signal_flag and why does nothing happen if it's set to False?
There was a problem hiding this comment.
Wait ok I think I understand what is happening here. You're using the helper _check_signal_rms to set this to True, and if it's not, you want to skip the fitting. A few things here:
- This is a great candidate for an early return, to avoid having all the extra indentation and the conditionals on
signal_flagbelow. - I don't understand why
_check_signal_rmsis setting a flag instead of just returning True or False - It doesn't look like
_check_signal_rmsis ever used elsewhere, and it's private. so why does it have an input parameter, instead of just internally accessingself.filtered_snr_data? Or alternatively just don't turnfiltered_snr_dataas an attribute, since it seems unused except right here?
|
|
||
| Parameters | ||
| ---------- | ||
| weight_sigma : float, optional |
There was a problem hiding this comment.
would it make sense for this attribute to always be scaled by del_nu, i.e., defined in terms of the number of resolution elements?
| if save_guess: | ||
| print('Saving initial model results to ', outfile) | ||
| print(self.lmfit_initial_guess_model.fit_report()) | ||
| save_modelresult(self.lmfit_initial_guess_model, outfile) |
There was a problem hiding this comment.
You're already saving the initial guess in an attribute, so would it make sense to just remove these lines, and then show in the example notebook how that attribute can be accessed after the fit? That would give the user more flexibility in what they want to do with the initial guess, e.g. print the report but not save it, do something with the initial guess in memory, etc?
| self.nu_low_bound = None | ||
| self.nu_high_bound = None | ||
|
|
||
| def fit_profile(self, |
There was a problem hiding this comment.
Overall I think this method could be made more flexible with a few small changes. I think you could make make_weights and make_initial_guess both public methods, and make them return their outputs instead of setting the outputs as attributes. Then you could make the weights and initial guess inputs to this fit_profile method. That would be more flexible because then a user could modify their initial guess or weighting scheme more easily.
This method could accept None for each of those, and generate a reasonable default if it encounters None.
| self.nu_at_Imax = self.x_to_fit[max_index] | ||
| self.Imax_value = self.y_to_fit[max_index] | ||
|
|
||
| if plot_pixel_fit and self.signal_flag: |
There was a problem hiding this comment.
If I'm reading the indentation correctly here, this is already only hit if the signal_flag is True
2 participants
Related to #66
I am going to not repeat everything in the issue above. When the PR is opened, I will attach a test script and notebook that has markdowns. How to share a large data file spectral cube for this is something TBD.