Add time-dependent iris_response function.#102
Conversation
|
Hi @asainz-solarphysics. I just merged the Slit Meta PR (#101) we reviewed this morning. This means you will have to do a |
|
Hi @asainz-solarphysics. My first comment should have been congratulations on your first PR! This should make collaborating on this functionality much easier. I will have a look over your code and make some suggestions. The first thing to do is to move the changes you've made in So long of this is done in your At first glance, it looks like the main changes are the new function , N.B. A word of advice, when on the command line in your local git directory, use |
DanRyanIrish
left a comment
There was a problem hiding this comment.
Hi @asainz-solarphysics. Here are couple suggestions to make your code more Pythonic and easier to read.
| if size_time_cal_coeffs[1] != 2 or size_cal_coeffs[1] < 2: | ||
|
|
||
| print('ERROR: Incorrect number of elements either in time_cal_coeffs or in cal_coeffs.') | ||
| return |
There was a problem hiding this comment.
The way you raise an error in Python is to replace the above two lines with
raise ValueError("Incorrect number of elements either in time_cal_coeffs or in cal_coeffs.")You can raise many different kinds of errors. The two most most are ValueError and TypeError. In this case the error is raised is one or two variables do not have a certain value. Therefore, we should raise a ValueError.
Also, once an error is raise, the program is stopped. Therefore, you don't need the else part of the statement. You can just exit the if statement entirely and carry on with the function, e.g.
if size_time_cal_coeffs[1] != 2 or size_cal_coeffs[1] < 2:
# Raise ValueError is time coefficient have the wrong format.
raise ValueError("Incorrect number of elements either in time_cal_coeffs or in cal_coeffs.")
# Exponent for transition between exp.decay intervals.
transition_exp = 1.5
...| # exponent for transition between exp.decay intervals | ||
| transition_exp = 1.5 | ||
| yr2sec = np.float64(365.25*24.*3600.) | ||
| out = np.zeros(len(list(time_obs)), dtype=np.float64) |
There was a problem hiding this comment.
It is generally better practice to give meaningful variable names. After all, this code will be read far more times than it is written and meaningful variable names make it easier for humans, especially those reading the code for the first time, to understand what is going on.
So I would suggest changing out to a more physically meaningful name.
|
Hi @asainz-solarphysics, I had a quick look through |
|
Thanks, Daniel! I appreciate a lot your suggestions, comments, and
corrections.
…On Tue, Aug 21, 2018 at 10:07 AM DanRyanIrish ***@***.***> wrote:
Hi @asainz-solarphysics <https://github.com/asainz-solarphysics>, I had a
quick look through fit_iris_xput. It looks fairly good! I've left a
couple comments about how to make it "better Python". I also see that this
function really needs a docstring, particularly to explain the format and
meaning of the coefficient inputs and outputs to make the code itself
easier to follow. I know you said that's on your to-do list so I look
forward to seeing it :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#102 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AioI9h339e_XW9hPGr1fRV2e3UAqP96yks5uTD5ogaJpZM4WEieu>
.
|
|
Hi @asainz-solarphysics. How are you getting on with this? Do you need any further help pushing new changes? |
|
Hi @asainz-solarphysics. What news on this PR? |
|
Hi @asainz-solarphysics. I haven't seen any further activity on this PR for a while. This is understandable considering other pressing priorities I know you have. However, since this functionality is still very important to IRISpy, I would like to make the next stages of this PR available for a student to take over if they get funded. Would you have any objection to this? If you would like to stay involved, you could act as IRIS team advisor while I can be the Python advisor. However, there's no obligation and I can work with the student myself. |
Adding fit_response function to iris_tools.py