Adding function sun_position_el (ported from SatelliteToolbox.jl)

[spacenewb]

Added sun_position_el and associated tests

[ronisbr]

Hi @spacenewb!

Thanks!

I have some comments that I will provide as a review in the source-code.

Furthermore, I think we should not add those functionalities in SatelliteToolboxBase.jl. Otherwise, packages that will not need all those functionalities will need to load them. The correct approach is to add SatelliteToolboxTransformations.jl as dependency to this package. Can you please make this modification?

[spacenewb]

Sure!

It was tardy on my part. I was too lazy to check the source code in SatelliteToolboxTransformations.jl. I have corrected the duplicate import issues now. There was a minor bug in the sun_position_el function as well, which is now corrected. On my local system, SatelliteToolboxCelestialBodies.jl now passes all the tests.

[spacenewb]

I changed the algorithm used by the function to compute the topocentric sun position. The new algorithm is called PSA+. It can be found at Updating the PSA sun position algorithm.

[ronisbr]

Hi @spacenewb!

I left some comments in the code review section, can you please take a look? The most important one is related the function return be linked to a flag. In this case, the code will be type unstable and thus very slow.

[spacenewb]

Hi @spacenewb!

I left some comments in the code review section, can you please take a look? The most important one is related the function return be linked to a flag. In this case, the code will be type unstable and thus very slow.

Hi @ronisbr

I cannot see any pending requests. I don't know if this is my mistake. Could you please check if you have left a code review

[ronisbr]

Yes! I think so, take a look here: https://github.com/JuliaSpace/SatelliteToolboxCelestialBodies.jl/pull/2/files

The main problem is related to the flag, because it will add type instability.

[spacenewb]

I have updated the function sun_position_el such that it returns all output variables without any flag. I also happened to stumble upon the need to use the radius of the Earth as a constant. Do you think it's wise to export it as a constant in the SatelliteToolboxBase and then use it here in SatelliteToolboxCelestialBodies?

[spacenewb]

Yes! I think so, take a look here: https://github.com/JuliaSpace/SatelliteToolboxCelestialBodies.jl/pull/2/files

The main problem is related to the flag, because it will add type instability.

I still cannot see any reviews. I am sorry because I am learning the platform. I must be doing something wrong.

I get this when I filter the PR list on the basis of reviews:

Reviews - Not reviewed by me:

Reviews - Changes requested:

However, I have addressed the main issue about the flag in sun_position_el.

[ronisbr]

Hum, very strange, I am seeing the reviews in the code. Do you mind if I merge this commit and make the modifications myself?

[spacenewb]

Please feel free to merge this commit. I am sorry that I could not do the modifications myself.