Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cleanup light curve notebook (complete Issue #177) #198
Cleanup light curve notebook (complete Issue #177) #198
Changes from 7 commits
3f9b0c0
a795eba
2395da3
83fe703
636d4c6
4c883b0
eac04c6
34a9296
dd617e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a little bit of inconsistency that the code uses pyvo yet refers to astroquery functionality to list the available missions.
(However this discrepancy should go away as the heasarc module is being reworked (yet I don't expect us to change back this code to use astroquery)
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low priority, but you may want to run flake8 (or autopep8) on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do that I'd rather we do it on all the files at the same time, and do it in it's own PR so that it's easier to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not recommend using ascii directly, but staying with the higher level Table API. There maybe other occurrences, I haven't explicitly looked for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, in general I'm a bit proponent of using full words for variables, even if they seem superfluous. This makes the code more accessible and readable. What about
results_table =
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the occurrences of
ascii.read
toTable.read
in this file.I'm good with full words for variable names, but if we're going to start changing pre-existing names like this one I'd rather do it in a separate PR and review/update all the files at the same time.
This file was deleted.