search

laserkelvin / PySpecTools

Routines for rotational spectroscopy analysis written in Python 3
MIT License
31 stars 5 forks source link

Update assignment.py - Friendlier Figures #18

Closed aowen-uwmad closed 4 years ago

aowen-uwmad commented 4 years ago
Enhanced .plot_spectrum()

Added option to choose whether intensity or SNR or both is being plotted. Added y-axis labels for either intensity or SNR plots.

Added .plot_spectrum2() for comparing subplots.

I wanted to be able to directly compare spectra in the same plot. Can choose to synchronize x-axes of subplots, for easier direct comparison. Can also choose the orientation of the subplots.

Comments, suggestions are welcome. (This is my first time doing fork & pull request.)

laserkelvin commented 4 years ago

Thanks a lot @aowen-uwmad!

Let me take a look and get back to you. The way that PRs should work is typically by first starting an issue, so we can discuss potential solutions to something you'd like to do. When you submit a PR, you can then reference the issue using "#issue_number", and helps keep track what PR was used to fix/implement what issue.

In this case, from a code organization point of view it's not so informative to have a method called plot_spectrum2, as it's not immediately obvious what it does on top of plot_spectrum. I would suggest that we have a method in the analysis module, and call it plot_experiments(*experiments), where the input is a chain of Session objects, and we make a single plot by iterating through each Session and overlaying the data. Do you want to try implementing something like this?

aowen-uwmad commented 4 years ago

Yes, it's not a good name. I wasn't really sure what to call it. Putting it in the analysis module as you suggested seems like the better idea. And I hadn't even considered comparing more than two experiments at a time, but that should be straightforward.

we make a single plot by iterating through each Session and overlaying the data

I think it depends on what you mean by "single plot" and "overlaying". If the spectrum of each experiment is being added as just another trace, there will almost certainly be issues where the magnitude of intensities is mismatched. That's where the subplots would be a better idea. We could have something like plot_experiments(experimentslist,inttype=,xsync=,), which for each Session object in experimentslist a subplot of intensity or SNR specified by inttype is created, and the x-axes of subplots are synced when xsync=True ?

And I will follow your suggestion of first submitting an issue. I didn't think of that since it's about adding a feature rather than fixing a bug.

aowen-uwmad commented 4 years ago

I'm going to close the pull request and resubmit as issues.