search

sunpy / sunraster

A SunPy-affiliated package which provides tools to analyze data from spectral data from any solar mission.
BSD 2-Clause "Simplified" License
20 stars 25 forks source link

Time-Dependent IRIS Effective Areas #27

Closed DanRyanIrish closed 4 years ago

DanRyanIrish commented 7 years ago

Enable time-dependent effective areas to be derived using get_iris_response() in iris_tools in the same way that it is done in SSW. See iris_get_response in SSW, a version of which can be found here: https://sohowww.nascom.nasa.gov/solarsoft/iris/idl/lmsal/util/iris_get_response.pro

kakirastern commented 5 years ago

Hi, I am interested in offering help on this issue if it's not yet taken?

DanRyanIrish commented 5 years ago

Hi @kakirastern. More help on this issue would be great as our initial work has stalled and I think it's fair to say we need another injection over effort to move it forward. I will set up a new branch to which we can make PRs to that maintains the work already done.

harsh7g commented 5 years ago

Hi, I would like to work on this issue so can someone help me out with it ?

kakirastern commented 5 years ago

Hi @iamharshgupta, I have expressed interest in this issue first so would like to give it a try...

DanRyanIrish commented 5 years ago

Hi @iamharshgupta. I'm thrilled to hear you are interested in contributing! We can always use more help! When it comes to this issue however, @kakirastern has already started working on it and I think it would be more efficient to have one person working on this issue at a time.

However, there are plenty of things to still get involved in. Would you be interested in helping with an issue in the ndcube package? ndcube is the package upon which IRISpy principally depends. I have one in mind: https://github.com/sunpy/ndcube/issues/149. If so, we could start a conversation on that issue page with @Cadair one how we should start going about it.

DanRyanIrish commented 5 years ago

@kakirastern I know I promised to set up a branch to which you can pull and open a PR to. I'll do that this evening and let you know when it's available.

kakirastern commented 5 years ago

@DanRyanIrish Yeah, been waiting for it... I appreciate your reply! Just let me know when it is ready.

harsh7g commented 5 years ago

Hi @DanRyanIrish , could you please give me your riot username so I could talk to you there ?

DanRyanIrish commented 5 years ago

Hi @kakirastern. I had permissions issues over the weekend which meant I couldn't create the new branch for you like I said. I've sorted that now though the new branch is time_dependent_response in the sunpy/irispy repo: https://github.com/sunpy/irispy/tree/time_dependent_response So you should checkout that branch in your own repo and start committing to it. Then push it to your own fork and when the time comes open a PR to that branch in the sunpy/irispy repo, not the master branch which is the default. I can talk you through this process on riot is you like.

kakirastern commented 5 years ago

Hi @DanRyanIrish. Yup, I would definitely need some kind of a walking through the process, as I haven't tried something similar before. But I will give it a try between now and when we meet on Riot say sometime during day time in EDT over the next few days to see if I can figure it out on my own first.

kakirastern commented 5 years ago

Just checked-out the branch https://github.com/sunpy/irispy/tree/time_dependent_response and have set up tracking upstream from my local branch to my own fork at https://github.com/kakirastern/irispy/tree/time_dependent_response Will start committing to it soon.

DanRyanIrish commented 5 years ago

Sounds like you've figured it out! The only other quirk will be setting up the PR. By default GitHub will probably issue the PR to the sunpy/irispy (base) master branch. That just needs to be changed in the drop-down menu to time_dependent_response when setting up the PR.

DanRyanIrish commented 5 years ago

The first thing to do is to look at the original PR and address the few inline comments I had left.

The next thing will be to move the relevant changes related to the time-dependent response feature from mi_iris_tools.py to iris_tools.py. You may be able to just replace the whole file, but I'll take a look and let you know if it's that simple.

Once that's done, we can take a closer line-by-line look at the code and come up with a list of tasks to make this code appropriate to be merged into irispy master.

DanRyanIrish commented 5 years ago

@kakirastern I had a quick look. It is as simple as deleting iris_tools.py and renaming mi_iris_tools.py to iris_tools.py.

DanRyanIrish commented 5 years ago

Also, the original IDL function that this project is rewritting is here. Don't worry if it's hard to understand, even if you know a bit of IDL. But comparing it with what Alberto has written in Python might be helpful in understanding what the code is supposed to do.

kakirastern commented 5 years ago

Yup, I have removed the original iris_tool.py file and replaced it with the renamed mi_iris_tools.py now known as iris_tool.py in PR #108.

DanRyanIrish commented 4 years ago

Achieved in PR #119