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 time-dependent iris_response function. #102

Merged
merged 1 commit into from Aug 20, 2019

Conversation

asainz-solarphysics
Copy link
Contributor

Adding fit_response function to iris_tools.py

@DanRyanIrish
Copy link
Member

Hi @asainz-solarphysics. I just merged the Slit Meta PR (#101) we reviewed this morning. This means you will have to do a git pull origin master on your my_response branch to make sure your branch keeps up to date and avoids conflicts with the sunpy/irispy master version.

@DanRyanIrish
Copy link
Member

DanRyanIrish commented Aug 20, 2018

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 mi_iris_tools.py into iris_tools.py. This will allow us to use the power of GitHub to easily compare your changes to what's already there.

So long of this is done in your my_response branch, it will not destroy any of the IRISpy code on the master branch. And for your own work, if you want to use the communal version of IRISpy, all you have to do is change back to the master branch on your computer using git checkout master.

At first glance, it looks like the main changes are the new function ,fit_iris_xput, and the code in the if iris_response["VERSION"] > 2: if statement in get_iris_response. Start with copying those into the iris_tools.py file on your my_response branch and then push your changes to you my_github remote.

N.B. A word of advice, when on the command line in your local git directory, use git branch and git status a lot to always double check what branch you're on, and the status of your changes. Don't forget to git add filename.py your changes before doing git commit.

@DanRyanIrish DanRyanIrish self-requested a review August 21, 2018 16:36
Copy link
Member

@DanRyanIrish DanRyanIrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DanRyanIrish
Copy link
Member

Hi @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 :)

@asainz-solarphysics
Copy link
Contributor Author

asainz-solarphysics commented Aug 21, 2018 via email

@DanRyanIrish
Copy link
Member

Hi @asainz-solarphysics. How are you getting on with this? Do you need any further help pushing new changes?

@DanRyanIrish
Copy link
Member

Hi @asainz-solarphysics. What news on this PR?

@DanRyanIrish
Copy link
Member

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.

kakirastern added a commit to kakirastern/sunraster that referenced this pull request Apr 10, 2019
kakirastern added a commit to kakirastern/sunraster that referenced this pull request May 24, 2019
@DanRyanIrish DanRyanIrish merged commit 11d4c93 into sunpy:master Aug 20, 2019
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.

None yet

2 participants