Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add ability to remove multiple rfi channels #17

Merged
merged 3 commits into from
Jul 3, 2021

Conversation

kevinawilson
Copy link
Contributor

This changes the expected rfi parameter for plot() from a two-item list to a list of tuples, each one of which represents the low and high frequencies of the band to filter from the plot.

I did not change the version number. The contributor guidelines said that this project uses SemVer for its versioning. In SemVer, any change to the API (like this) requires a new major version number, but you had indicated that backward compatibility was not an issue, so I didn't know if you wanted to make this a minor version increase instead.

I didn't see anything in the README.md that needed to be changed, but obviously it will require an update to the documentation.

@0xCoto
Copy link
Owner

0xCoto commented Jul 1, 2021

Thanks a lot for the PR! Would it be possible to update the .rst in the docs as well (particularly the examples and references page) to ensure everything is up-to-date?

As for the versioning, I'd probably consider it a minor release, but a major update is also quite arguable, so feel free to change setup.py based on your gut, and I'd be happy to create a new release (push the update to PyPi).

@kevinawilson
Copy link
Contributor Author

I can change the .rst files. I'll try to do that this evening.

I have a couple of other things I would like to add, such as the fixed axes for plots. Perhaps you could wait until those are in place to do a major version change.

By the way, do the plots measure simple relative velocity or are they corrected to VLSR? I was discussing my plots on the SARA list serve, and someone noticed that my plots were blue shifted with respect to the expected results. Astropy has a new module that can change the frame of reference from topocentric to VLSR. If you aren't already adjusting for this, perhaps I can add that (with a boolean parameter in plot() to indicate whether to transform the spectrum to the VSLR).

@0xCoto
Copy link
Owner

0xCoto commented Jul 1, 2021

I appreciate your interest in contributing to the software Kevin, you've got plenty of good suggestions that I too would like to see being implemented into Virgo. Feel free to take your time and submit a more complete version whenever you wish, with the additions you like.

The plots calculate radial (relative line-of-sight) velocity indeed. Supporting VLSR correction would be awesome to see though. I'm aware you can do such computations with Astropy but never really got to it, although I definitely see the need for this, so if you could also apply this feature as something like plot(vlsr=True/False), that would be perfect! You could also adjust the plot title of the spectra to indicate if VLSR has been corrected (i.e. adjust "Radial Velocity (km/s)") based on the boolean value of the parameter.

@kevinawilson
Copy link
Contributor Author

I've added the information to the documentation and setup.py. It is ready for review.

@0xCoto
Copy link
Owner

0xCoto commented Jul 3, 2021

Is there a typo in the second RFI range at Line 35? Low frequency appears to be higher than the high frequency. Other than that, everything looks good!

@kevinawilson
Copy link
Contributor Author

The typo has been fixed.

@0xCoto 0xCoto merged commit db64b19 into 0xCoto:master Jul 3, 2021
@0xCoto 0xCoto mentioned this pull request Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants