Proof of concept: SGP4 submodule

[crbinz]

This is sort of a weird PR - I hope it is not unwelcome. It addresses one of my suggestions in #1 - breaking out your SGP4 implementation as a standalone package. In this PR, I have rearranged the code such that the SGP4 functionality all lives inside the SatelliteToolbox.SGP4 submodule, in order to demonstrate how it could be broken out as an entirely separate project in the future.

I hope it is a fairly clean change, with one exception: the functionality in SGP4/src/gmst.jl has been copied from SatelliteToolbox/src/transformations/gmst.jl, as I couldn't figure out a great way to avoid that. Existing tests pass without modification.

Please let me know if you have any questions or suggestions. I think a standalone, pure-Julia implementation of SGP4 would be very valuable and useful to the community.

[coveralls]

Pull Request Test Coverage Report for Build 207

• 4 of 5 (80.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+2.4%) to 66.777%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/submodules/SGP4/SGP4.jl	0	1	0.0%
<!--	Total:	4	5	80.0%	-->

Totals
Change from base Build 199:	2.4%
Covered Lines:	9644
Relevant Lines:	14442

---

💛 - Coveralls

[codecov-io]

Codecov Report

Merging #33 into master will increase coverage by 0.02%. The diff coverage is 90%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   73.51%   73.53%   +0.02%     
==========================================
  Files          74       76       +2     
  Lines        3749     3756       +7     
==========================================
+ Hits         2756     2762       +6     
- Misses        993      994       +1

Impacted Files	Coverage Δ
src/SatelliteToolbox.jl	100% <ø> (ø)	:arrow_up:
src/types.jl	100% <ø> (ø)	:arrow_up:
src/submodules/SGP4/gmst.jl	100% <100%> (ø)
src/submodules/SGP4/tle.jl	56.56% <100%> (ø)
src/submodules/SGP4/sgp4_model.jl	99.31% <100%> (ø)
src/submodules/SGP4/SGP4.jl	50% <50%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c6abac4...e8b4506. Read the comment docs.

[ronisbr]

Hi @crbinz !

Thanks for the PR, now I know how can we use submodules :D

I am still not convinced when or how we should split a package into multiple packages (I was strongly advised against this by some Julia devs in early stages of development). However, I think using submodules is a good path towards this modification. Maybe @helgee can provide some thoughts about it too?

About this PR:

1. I think I do not like having submodules inside random folders in ./src, because it will be very messy. I propose to add the submodules inside the folder ./src/submodules/. Thus, SGP4 will be in ./src/submodules/SGP4. In this case, it will be very clear which parts are candidates to become new packages.
2. SatelliteToolbox.jl must work transparently to the user. I tried your branch, and I got many errors in a simulator we have here (did not have time to verify why). Probably because the symbols of SatelliteToolbox.SGP4 are not being reexported. I really do not want to do using SatelliteToolbox, SatelliteToolbox.SP4.
3. We must try to avoid at all cost code duplication, because it can be very hard to maintain it in the future. However, I do not have a good proposal to avoid the duplication of the function JDtoGMST right now.
4. In specific case of SGP4, we have two thing to solve. First, there is already a package called SGP4, and second we must also create the TLE sub package. Notice that the TLE can be used in other propagators. Thus, having TLE definition inside SGP4 is not a correct move IMHO.

Can you modify your PR to address those comments?

[ronisbr]

Ah! One more thing, can we have the tests inside the submodule? Something like ./src/submodules/SGP4/tests. We will need this in a package split.

[helgee]

1. I think I do not like having submodules inside random folders in ./src, because it will be very messy. I propose to add the submodules inside the folder ./src/submodules/. Thus, SGP4 will be in ./src/submodules/SGP4. In this case, it will be very clear which parts are candidates to become new packages.

This seems a bit over-the-top for my taste. What I like to do is to have a folder for each submodule (src/lowercase_module_name) and then the corresponding files inside of it (src/lowercase_module_name/UppercaseModuleName.jl), e.g. src/earth_attitude/EarthAttitude.jl.

This way you know which files belong to which module but you do not have a generic submodules folder.

For tests I use a similar approach: Either one file per submodule or a folder if there is more than one, e.g. test/earth_attitude.jl or test/earth_attitude/runtests.jl.

[ronisbr]

Hi @helgee

Thanks for your feedback!

After analyzing the folder structure of SatelliteToolbox.jl, I am afraid that if all submodules go to a directory inside ./src/ it will be very crowded very soon. Currently, we have 8 folders inside ./src, there is at least 9 submodules I can think of: constants, IGRF, NRLMSISE-00, JR1971, JB2008, gravity models, ICGEM, TLE, SGP4 (maybe we can merge all the atmospheric models inside a single package...). Thus, I think I still prefer the structure:

./src/submodules/SGP4.jl/ ./src/submodules/IGRF.jl/

and so on.

For the tests, I think we can do the same:

./test/submodules/SGP4.jl/<test files> ./test/submodules/IGRF.jl/<test files>

The idea is to make things very simple to eventually split the package.

@crbinz can you please modify your PR to match this structure (together with my past comments)?

[crbinz]

Thanks for the feedback. Obviously, how you want to organize your projects is completely up to you, but I know that I am more likely to use a package if it offers some singular functionality that I need with a very basic API that doesn't take much to figure out. I think the SGP4 model is broadly useful to the community, and others will want to use it, and so that's why I made this proposal.

• I'm happy to organize this however you'd like. I want to make clear, though, that I intend this to be an intermediate step towards the ultimate goal of just splitting off SGP4 as its own package.
• I'm sorry about the errors, I was only running the tests to make sure they passed. I'll take a closer look.
• I'm open to ideas about how to handle JDtoGMST that do not involve code duplication.
• With the exception of the PPT3 model (which almost no one knows about, let alone uses) working with its own type of TLE, what other propagators can use TLEs? Broadly, the use of a TLE implies the use of SGP4.
• The existing SGP4.jl is mine, an is just a basic wrapper of the python sgp4 implementation. I don't remember what the latest is on allowing multiple packages with the same name in the General registry, but I think it is supposed to be possible? Otherwise, I'm happy to help with whatever steps need to be taken to make this package SGP4.jl and either deprecate the other one completely or name it something like PySGP4.jl.

I will try to get to the changes when I can, but I cannot promise exactly how soon that will be. Hopefully within a week or so.

[ronisbr]

• I'm happy to organize this however you'd like. I want to make clear, though, that I intend this to be an intermediate step towards the ultimate goal of just splitting off SGP4 as its own package.

Good, I think this is the right direction.

• I'm sorry about the errors, I was only running the tests to make sure they passed. I'll take a closer look.

It's OK! :) I make some tests and probably Reexport.jl will fix all of them.

• I'm open to ideas about how to handle JDtoGMST that do not involve code duplication.

Perhaps we have no solution here. Let's just duplicate this file, it is a small function anyway.

• With the exception of the PPT3 model (which almost no one knows about, let alone uses) working with its own type of TLE, what other propagators can use TLEs? Broadly, the use of a TLE implies the use of SGP4.

There is the SGP8, which I will eventually add to SatelliteToolbox. Furthermore, maybe the user wants just to "unpack" the TLE and not propagate the orbit.

• The existing SGP4.jl is mine, an is just a basic wrapper of the python sgp4 implementation. I don't remember what the latest is on allowing multiple packages with the same name in the General registry, but I think it is supposed to be possible? Otherwise, I'm happy to help with whatever steps need to be taken to make this package SGP4.jl and either deprecate the other one completely or name it something like PySGP4.jl.

AFAIK, you can have multiple packages with the same name but not both on General registry.

Maybe the right thing to do to all packages derived from SatelliteToolbox.jl is to name them like SatelliteToolboxSGP4, or something. More or less to what they do in OrdinaryDiffEq (DiffEq*). This will prevent a lot of naming problems. What do you think?

I will try to get to the changes when I can, but I cannot promise exactly how soon that will be. Hopefully within a week or so.

Nice!

[crbinz]

There is the SGP8, which I will eventually add to SatelliteToolbox.

Good point, I hadn't noticed that your model doesn't implement SGP8. According to the Wikipedia page (and consistent with my own understanding), the entire set of SGP/SGP4/SDP4/SGP8/SDP8 "is often referred to collectively as SGP4", so I would not expect to find an SGP8 package.

Furthermore, maybe the user wants just to "unpack" the TLE and not propagate the orbit.

Unpacking directly (e.g. just parsing) would yield Brouwer mean elements derived from the SGP4 model, which would only be usable with SGP4 anyway. Converting to, e.g., Cartesian position and velocity also requires the use of SGP4.

Maybe the right thing to do to all packages derived from SatelliteToolbox.jl is to name them like SatelliteToolboxSGP4, or something. More or less to what they do in OrdinaryDiffEq (DiffEq*). This will prevent a lot of naming problems. What do you think?

I don't necessarily mind this, but if it's only to avoid the naming issue, I am not sure that's a sufficient reason. On the other hand, if it implies easy interoperability within the SatelliteToolbox* ecosystem (similar to the DifferentialEquations.jl ecosystem), then I'd say that would make sense.

[ronisbr]

Good point, I hadn't noticed that your model doesn't implement SGP8. According to the Wikipedia page (and consistent with my own understanding), the entire set of SGP/SGP4/SDP4/SGP8/SDP8 "is often referred to collectively as SGP4", so I would not expect to find an SGP8 package.

This is not very good since those are somewhat different algorithms. IMHO it is better to have both SGP4.jl package and SGP8.jl or a SGP.jl that has both.

Unpacking directly (e.g. just parsing) would yield Brouwer mean elements derived from the SGP4 model, which would only be usable with SGP4 anyway. Converting to, e.g., Cartesian position and velocity also requires the use of SGP4.

Sometimes we want to check if the TLE was correctly uploaded to our satellites (this is just one example). The telemetry fields usually are the TLE information but “unpacked”. In this case, the functionality of SatelliteToolbox can be used to see those fields to easily compare to the telemetry.

I don't necessarily mind this, but if it's only to avoid the naming issue, I am not sure that's a sufficient reason. On the other hand, if it implies easy interoperability within the SatelliteToolbox* ecosystem (similar to the DifferentialEquations.jl ecosystem), then I'd say that would make sense.

OK, so let’s keep all packages with SatelliteToolbox prefix to imply it is part of the SatelliteToolbox ecosystem (we can even use SatToolbox if the name gets too big. For example, the next candidates are the atmospheric models, in this case I think we should use SatToolboxAtmosphericModels).

[ronisbr]

Hi @crbinz

Just one tip, probably the tests are failing because of the file Manifest.toml, which must not be added.

[ronisbr]

@crbinz, I am not sure if it is being caused by the mobile browser I am using right now, but it seems that when you copied the SGP4 model, you used an older (slower) version.

[crbinz]

Thanks, I'll take a look at both of those, and continue addressing the rest of the suggestions.

[crbinz]

I think I have now addressed everything except the name of the submodule (because I'm not sure if it's the right time to change that or not), and the separate TLE submodule (since, perhaps, that should be in a separate PR?). Please let me know what you think.

[ronisbr]

Hi @crbinz

Thanks! I think we can let TLE inside SGP4 for now, I can move it to a separate module.

The name is also fine, we should rename only when we split the package.

However, the problem is that the modification must be transparent to the user of SatelliteToolbox. Hence, the tests files must not change at all (except from the SGP4 files).

Furthermore, I am getting a problem related to duplicated functions, probably caused by the duplicated file. The solution should be to not export the duplicated functions in SGP4 submodule.

[crbinz]

Thanks for the feedback (and your patience)! I unmodified the tests, and stopped exporting the functions in gmst.jl.

[ronisbr]

Perfect! Seems nice now :)

Thanks for your contributions!

[ronisbr]

Done! I have moved TLE to another module. In this case, I needed to call it SatelliteToolboxTLE so that it does not interfere with the TLE structure name.

By the way, I also needed to modify the SGP4 submodules tests because it was using the API functions and the submodule tests should be self-contained if we want to move them to another package.