Pull Request / Code review #3
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vditomasso
reviewed
May 25, 2017
| @@ -1,182 +1,221 @@ | |||
|
|
|||
| # Jean-paul Ventura | |||
| # January 12, 2017 | |||
| # Modified: February 20th, 2016 | |||
There was a problem hiding this comment.
It's not clear what the significance of these dates are and I'm not sure that you need them because GitHub tracks when you change things.
| This function measures the equivalent width of spectral emission/absorption features. | ||
|
|
||
| ==================== | ||
| Function parameters: |
There was a problem hiding this comment.
Is there a reason this is "Function parameters" and not "Arguments"?
|
|
||
| wavelength2 - wavelength value where continuum region to the left of the spectral feature ends. | ||
| # Define wavelength and flux arrays by accessing .fits sloan data |
There was a problem hiding this comment.
Does this code only work for sloan data? If so, include that in the description of the function in your docstring/readme
| fitl = np.array(m*lmbda + c) | ||
|
|
||
| # Normalize the flux array by dividing it by the fitline. | ||
| nflux = flux/fitline |
There was a problem hiding this comment.
Where is the variable "fitline" defined?
| # Sum continuosly over the normalized flux array and assign result to a variable. | ||
| eqwi = sum(1-nflux[ewindx])*dlmbda | ||
|
|
||
| # Calculate the uncertainty in the equivalent with measurement |
|
|
||
| """ | ||
| # assign the number of monte carlo iterations to variable 'size'. | ||
| size = 3000 |
There was a problem hiding this comment.
"iter" might be a better variable name than "size"
| #Calculate line height of spectral emission feature | ||
| def line_height(filename,wavelength1,wavelength2,wavelength3,wavelength4): | ||
| """ | ||
| Tnis function calculates the emission line feature height as the vertical-distance between the |
| # Isolate the wavelength subarrays pertaining to both of the wings of the line feature | ||
| x1 = lmbda[coindx] | ||
|
|
||
| #Isolate the normalized flux subarrays of the line features wings. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I can't find Victoria to initialize the code review... Only options listed are Kelle Cruz and Eileen Gonzalez.