Removes unused variable zloop in trace.py

[Ghaithq]

This resolves issue #196.
Removed variable and adjusted code accordingly including tests.

[nabobalis]

I would like to know if this was added by mistake or its actually needed by the original algorithm and we didn't code it correctly.

[Ghaithq]

I would like to know if this was added by mistake or its actually needed by the original algorithm and we didn't code it correctly.

Ok. I am going through the paper. Once I am sure I will inform you.

[Ghaithq]

I would like to know if this was added by mistake or its actually needed by the original algorithm and we didn't code it correctly.

From what I understand, in the paper the z value(which I think is called zl in the code) is used to refer to intensity(flux) which is used for internal computations. However, the variables xloop, yloop and zloop are variables that are returned from the function and internal calculations are independent of them. the author of the file passed the value zloop to _loop_add() but didn't use it at all. In the paper, The value zloop or anything resembling it didn't exist. That's from what I understand which might not be correct. In conclusion, in the pre-modified function the variable is completely useless.

[Ghaithq]

If this pull requested is accepted. There's recommended values for the parameters in the function which we can use as default parameters.

[nabobalis]

If this pull requested is accepted. There's recommended values for the parameters in the function which we can use as default parameters.

Default values for?

[Ghaithq]

If this pull requested is accepted. There's recommended values for the parameters in the function which we can use as default parameters.

Default values for?

the parameters of the occult2 function.

[nabobalis]

If this pull requested is accepted. There's recommended values for the parameters in the function which we can use as default parameters.

Default values for?

the parameters of the occult2 function.

Which are?

[Ghaithq]

If this pull requested is accepted. There's recommended values for the parameters in the function which we can use as default parameters.

Default values for?

the parameters of the occult2 function.

Which are?

𝑛𝑠𝑚1=1
, 𝑛𝑠𝑚2=3
, 𝑟𝑚𝑖𝑛=30

[nabobalis]

I see no harm in adding them.

[wtbarnes]

I'm confused. What do these additional parameters have to do with the original scope of the PR?

[nabobalis]

I'm confused. What do these additional parameters have to do with the original scope of the PR?

Nothing. They can be done in a follow up PR.

[Ghaithq]

I'm confused. What do these additional parameters have to do with the original scope of the PR?

I can leave it as is for now. Adding the default values will change the order of parameters, which might be annoying if anyone is using the function.

[nabobalis]

I'm confused. What do these additional parameters have to do with the original scope of the PR?

I can leave it as is for now. Adding the default values will change the order of parameters, which might be annoying if anyone is using the function.

Why would we need to change the order?

[nabobalis]

Thanks for the PR @Ghaithq

[Ghaithq]

I'm confused. What do these additional parameters have to do with the original scope of the PR?

I can leave it as is for now. Adding the default values will change the order of parameters, which might be annoying if anyone is using the function.

Why would we need to change the order?

Because default arguments need to come at the end. and the parameters nsm1, rmin are not at the end.

[wtbarnes]

I do not think that simply removing the zloop variable was the right choice. If you look at the OCCULT2 source, the flux at the traced points is written to a file along with the loop coordinates in pixel space. Rather than just removing this variable, we should understand how to use it to return the flux along the traced loop or alternatively we could add a gallery example that shows how to get the flux at the points along the traced loop using sunpy.

[nabobalis]

If it was only used to save out, then I think adding to the example how to get the flux along the loop is what I would lean towards.

[nabobalis]

But if we want to do that, will we need this commit to be reverted?

[wtbarnes]

No

[wtbarnes]

My point is that we should consider more carefully what the intended functionality of these things before just dumping them. This variable is not "completely useless" as stated above.

[Ghaithq]

My point is that we should consider more carefully what the intended functionality of these things before just dumping them. This variable is not "completely useless" as stated above.

zloop was not returned in the previous function. it was only computed but never used or returned, which made me think it was useless given the situation. If you would like I can work on returning zloop along xloop and yloop.

[nabobalis]

My point is that we should consider more carefully what the intended functionality of these things before just dumping them. This variable is not "completely useless" as stated above.

Makes sense.
In that case, how do we want to go forward?

[nabobalis]

In that case, we should expand the example for this to:

"I think we should just use the coord tracing stuff we have in sunpy to show how once you have the SkyCoord you can extract the intensities a la https://docs.sunpy.org/en/stable/generated/gallery/units_and_coordinates/map_slit_extraction.html#sphx-glr-generated-gallery-units-and-coordinates-map-slit-extraction-py"

I shall open a tracking issue.