Add CR3BP functions created by Dhruv Jain #1475
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks @DhruvJ22 for this pull request! 👏🏽
There seem to be some quality issues with the code, have a look at the output of tox -e check (or here) and please fix accordingly.
The most pressing question is how to combine this with the existing CR3BP functionality that @JackCrusoe47 added in #1451 and that lives under https://github.com/poliastro/poliastro/tree/main/contrib/CR3BP. It is quite evident that there is some overlap between the packages, and it would be very inefficient to have similar implementations of the same thing. We should figure out how to move forward in a way that works for everybody.
I left two quick comments on the contrib/cr3bp_DhruvJ/cr3bp_char_quant.py module, but didn't make an in-depth review of the code. It seems to me that some things are new with respect to @JackCrusoe47 work, like the libration points and the Jacobi Constant, and others are already present there, like the integration of the equations of motion.
@JackCrusoe47 would you like to give this a more detailed review and leave some comments?
| # Body values | ||
| mu = {} # km^4 kg^-1 s^-2 | ||
| mu["Sun"] = 132712440017.99 | ||
| mu["Moon"] = 4902.8005821478 | ||
| mu["Earth"] = 398600.4415 | ||
|
|
||
| mu["Mars"] = 42828.314258067 # Mars, GM | ||
| mu["Jupiter"] = 126712767.8578 # Jupiter, GM | ||
| mu["Saturn"] = 37940626.061137 # Saturn, GM | ||
| mu["Uranus"] = 5794549.0070719 # Uranus, GM | ||
| mu["Neptune"] = 6836534.0638793 # Neptune, GM | ||
| mu["Pluto"] = 981.600887707 # Pluto, GM | ||
|
|
||
| mu["Phobos"] = 0.0007112 # Phobos, GM | ||
| mu["Titan"] = 8978.1382 # Titan, GM | ||
| mu["Ganymede"] = 9887.834 # Ganymede, GM | ||
| mu["Titania"] = 228.2 # Titania, GM | ||
| mu["Triton"] = 1427.598 # Triton, GM | ||
| mu["Charon"] = 102.30 # Charon, GM |
There was a problem hiding this comment.
Should this use poliastro.constants instead?
There was a problem hiding this comment.
yes, I am looking into leveraging that function to more seamlessly incorporate this code to poliastro/src in the future.
| ############# | ||
| distances = {} # km | ||
| distances["EarthMoon"] = 384400 | ||
| distances["SunEarth"] = 149600000 | ||
|
|
||
| distances["SunMars"] = 227944135 | ||
| distances["SunJupiter"] = 778279959 | ||
| distances["SunSaturn"] = 1427387908 | ||
| distances["SunUranus"] = 2870480873 | ||
| distances["SunNeptune"] = 4498337290 | ||
| distances["SunPluto"] = 5907150229 | ||
|
|
||
| distances["MarsPhobos"] = 9376 | ||
| distances["JupiterGanymede"] = 1070400 | ||
| distances["SaturnTitan"] = 1221865 | ||
| distances["UranusTitania"] = 436300 | ||
| distances["NeptuneTriton"] = 354759 | ||
| distances["PlutoCharon"] = 17536 |
There was a problem hiding this comment.
I suppose these are mean distances?
There was a problem hiding this comment.
Yes. they are all the mean distances used for CR3BP models.
There was a problem hiding this comment.
Thanks, @JackCrusoe47! Yes, these are mean distances obtained from the JPL references stated at the top of the code. @astrojuanlu does poliastro have the mean distances saved anywhere? If not, do you think we should look into adding these constants to constants.py like file.
| Non-dimensional time of P1-P2 system | ||
| """ | ||
| # Body values | ||
| mu = {} # km^4 kg^-1 s^-2 |
There was a problem hiding this comment.
small correction on units of mu: km^3 kg^-1 s^-2
There was a problem hiding this comment.
Thanks Kevin! I will update this typo.
| for a Cirular Restircted Three Body Problem (CR3BP) model setup | ||
|
|
||
| Features: | ||
| 1. Compute CR3BP 'mu', characterisitic mass of P1-P2 primary bodies |
There was a problem hiding this comment.
If I understand correctly, characteristic mass is M* = M1 + M2
and the right term for mu should be mass ratio or CR3BP constant. Could you double-check this?
There was a problem hiding this comment.
yup, you are right, my bad.
|
I personally really like this CR3BP library and prefer this over my implementation. If @DhruvJ22 is planning to add all other features like:
I think @DhruvJ22 would be much better suited to implement this library since you are currently working with Prof. Howell's research group. Amazing job 🚀 |
|
@JackCrusoe47 @astrojuanlu Thanks for all your comments and helping me! Kevin, if you are still interested in the development of CR3BP for poliastro then I would really appreciate your help, and we can talk about specific details on how we can build it together. |
|
@astrojuanlu, I am getting a cricleci: quality tox error and I don't have any idea on how to resolve it. I would really appreciate your guidance. Thank you! |
Codecov Report
@@ Coverage Diff @@
## main #1475 +/- ##
=======================================
Coverage 91.91% 91.91%
=======================================
Files 98 98
Lines 4500 4500
Branches 431 431
=======================================
Hits 4136 4136
Misses 274 274
Partials 90 90 Continue to review full report at Codecov.
|
|
Hi @DhruvJ22 Most errors can be fixed using the But in my last pull, some issues like trailing whitespace and other small errors, especially in the docstring, only worked last time after I manually modified my python files. I used the tox error output line and word number to identify the faulting character and removed it. Hope this helps. |
|
I would say, manually fixing these errors in your python files is easier.
|
…_cr3pb_DJ Updated local branch
for more information, see https://pre-commit.ci
|
@JackCrusoe47, thanks a lot for sharing your experience! |
…_cr3pb_DJ Updated local 2
|
@astrojuanlu , All checks passed!!! |
There was a problem hiding this comment.
Thanks for working on this, @DhruvJ22.
I left some nitpicks for improving the code-quality. In addition, let me ask for some additional features:
-
Would it be possible to include a
ReferencesorNotessection in the docstrings of the functions? I know you cited some books and manuals, but if we could also have the equation number attach that would be awesome. -
I bet we can move some of those contants to the main poliastro code by opening a pull-request only for this task. This way, you would also have some of your code included in the
mainbranch of the project.
From my point of view, and knowing that is is expected to be included under the contrib/ directory, I think it is ready to go.
I will educate myself in the CR3BP problem so we can look for ways of merging this into the main code.
| ax.set_xlabel("x [nd]") | ||
| ax.set_zlabel("z [nd]") | ||
| plt.legend() | ||
| pltnum = pltnum + 1 |
There was a problem hiding this comment.
Is this last command required? Since this is the end of the script, I am not sure how useful it is to add a unit to this variable.
Also, it may be interesting to add a plt.show() here, since this ensures that the graphics are displayed in the screen.
| import matplotlib.pyplot as plt | ||
| from cr3bp_char_quant import bodies_char | ||
| from cr3bp_lib_JC_calc import JC, lib_pt_loc | ||
| from cr3bp_master import prop_cr3bp |
There was a problem hiding this comment.
Just some formatting in the imports:
| import matplotlib.pyplot as plt | |
| from cr3bp_char_quant import bodies_char | |
| from cr3bp_lib_JC_calc import JC, lib_pt_loc | |
| from cr3bp_master import prop_cr3bp | |
| import matplotlib.pyplot as plt | |
| from cr3bp_char_quant import bodies_char | |
| from cr3bp_lib_JC_calc import JC, lib_pt_loc | |
| from cr3bp_master import prop_cr3bp |
| """Compute first-derivateive of pseudo-potenital terms of the CR3BP EOMs and acceleration terms | ||
| Dhruv Jain, Jan 10 2022 |
There was a problem hiding this comment.
| """Compute first-derivateive of pseudo-potenital terms of the CR3BP EOMs and acceleration terms | |
| Dhruv Jain, Jan 10 2022 | |
| """Compute first-derivateive of pseudo-potenital terms of the CR3BP EOMs and acceleration terms. | |
| Dhruv Jain, Jan 10 2022 |
| States are defined about the barycenter of the two primaries, P1 and P2 | ||
| """ |
There was a problem hiding this comment.
| States are defined about the barycenter of the two primaries, P1 and P2 | |
| """ | |
| States are defined about the barycenter of the two primaries, P1 and P2 | |
| """ |
| 5. Heteroclinic transfer design using Tau-Alpha method | ||
| 6. Method to transition and corret in N-body ephemeris model | ||
|
|
||
| My dream is to build enough features to enable me to include a targeter that I have created to compute Quasi-Periodic Orbits(QPOs) in CR3BP, so that users can access the less investaged but more useful QPOs. |
|
|
||
| Parameters | ||
| ---------- | ||
| mu : float, M2/(M1+M2) |
There was a problem hiding this comment.
Only supported types can be included within docstrings. If you would like to include the mathematics behind the variable, you can do so in the description of the argument, right below this line:
| mu : float, M2/(M1+M2) | |
| mu : float |
There was a problem hiding this comment.
Apply to the rest of docstrings in your code.
| t_node : list, float | ||
| Stores the time from one node to the next, t_node[-1]: time to reach the desired state (somekind of corrsing) | ||
| """ | ||
|
|
There was a problem hiding this comment.
First line of code must start right below the docstring. No need to have this blank line.
| Objective: This file contains functions to compute various characteristic quantities | ||
| for a Cirular Restircted Three Body Problem (CR3BP) model setup | ||
|
|
||
| Features: |
There was a problem hiding this comment.
There is no need to indent these lines.
| """Calculate mu of CR3BP | ||
| Parameters |
There was a problem hiding this comment.
Remember to add a blank line between the one-summary line and the rest of the sections.
There was a problem hiding this comment.
Applies to the rest of the docstrings in this module.
| if temp1 == temp2: | ||
| print( | ||
| "Same bodies passed as P1 and P2. Please pass the secondary body of P1 as P2" | ||
| ) | ||
| return 0, 0, 0 | ||
| except KeyError: | ||
| print("KeyError-> Incorrect/Does Not Exist Input Bodies name") | ||
| return 0, 0, 0 |
There was a problem hiding this comment.
Another nitpick. Although very useful for quick debugging, printing is not the most "professional" way of retrieving information about the status of a program.
As an alternative, consider raising logging, raising warnings or even errors.
Remember that you can customize the message of an error in Python by doing something like:
raise ValueError("Please, pass the right parameters...")
|
@jorgepiloto Thanks a lot for all your suggestions! I wish to mention that this is an older PR, so since this, I have made many changes and pushed some code to the main (#1570 ) with the help of Juan. I think it will be easier to review and make changes to smaller pieces of code as I add them to the main. Also at a macro level, the code in contrib might require some refactoring to be backward and forward-compatible. I will incorporate these suggestions as I push the particular file to the main. The most recent PR that I need help with is: #1573 |
I want to contribute to Poliastro by adding the CR3BP capability and implementing the things I have laid out in the README.