tudat-team / tudat-space

An introduction and guide to tudat-space.
https://tudat-space.readthedocs.io
14 stars 17 forks source link

Split asteroid orbit optimization example in three parts. #88

Closed sbcowan closed 1 year ago

sbcowan commented 1 year ago

The Asteroid Orbit Optimization example is split into three parts that build on each other. I changed the toc in examples.rst so that now the three files are given and not duplicated. I haven't figured out how to add a directory to a Jupyterlab only sphinx directory; this would allow a foldable directory for the Asteroid Orbit Optimization example.

I left the old complete file in the source, if one would want to look at that or revert easily.

DominicDirkx commented 1 year ago

Hi Sean, thanks! For now we can keep the structure of the examples as you have them. Could you open an issue here to remind us to look into the foldable directory for the asteroid example?

Below a few small comments and questions on the examples:

sbcowan commented 1 year ago

Perhaps a simple state propagation could be added to the first one

I think this is a good solution to the short-vs-very-long example problem. This would probably mean moving the Optimisation problem formulation section in the DSE part of the example to the Custom Environment part. One issue with this is that the AsteroidProblem class is used, and already introducing this in the Custom Environment part would be exceeding the reach of that example. I think this is the best solution possible, but it wouldn't result in the simplest state propagation.

I think it would flow better without direct mention of decision variables, objectives (but only for the first example!)

Agreed. I will implement this.

I forgot if we discussed it previously, but could this not be moved to tudatpy itself (was the issue the difficulty in testitng)?

I have done this, a pull request is imminent, however I am in the process of verifying that the tudatpy build works. I have tried building the bundle with the changes, but it didn't work. If the current build also fails, I will report back to you for assistance. I can still open an issue if you want, but as posted in Slack, I cannot assign myself yet as 'Assignee' so I didn't want to waste anyones time.

I think the optimization example would be more effective with all the stuff for design space exploration removed (the two are never really merged in plotting right?)

The two are indeed never really explicitly linked, so this would make the example shorter (assuming the issue on importing Jupyter notebooks doesn't get resolved shortly). I do think that, maybe in the future, it would be good to link them explicitly by explaining that certain bounds were chosen based on the findings of the DSE. If we completely decouple them, I think many users will brush over the DSE part of the example. I am unsure if that makes a difference. Let me know and I can implement this change as well.

DominicDirkx commented 1 year ago

I think this is a good solution to the short-vs-very-long example problem. This would probably mean moving the Optimisation problem formulation section in the DSE part of the example to the Custom Environment part. One issue with this is that the AsteroidProblem class is used, and already introducing this in the Custom Environment part would be exceeding the reach of that example. I think this is the best solution possible, but it wouldn't result in the simplest state propagation.

That's true, but I think it's fine. For those who want to know how to set up a custom environment, they can look at the first half, which is there. Adding the problem class and propagation may also be a good introduction into writing a problem class for an optimization with Tudat in the loop.

I have done this, a pull request is imminent, however I am in the process of verifying that the tudatpy build works. I have tried building the bundle with the changes, but it didn't work. If the current build also fails, I will report back to you for assistance. I can still open an issue if you want, but as posted in Slack, I cannot assign myself yet as 'Assignee' so I didn't want to waste anyones time.

Thanks, I wasn't sure which of these functions were on the short-term list of incorporation into tudatpy. I think if I change your github rights, you can add yourself as assignee. I'll look into it

The two are indeed never really explicitly linked, so this would make the example shorter (assuming the https://github.com/tudat-team/tudat-space/issues/87 on importing Jupyter notebooks doesn't get resolved shortly). I do think that, maybe in the future, it would be good to link them explicitly by explaining that certain bounds were chosen based on the findings of the DSE. If we completely decouple them, I think many users will brush over the DSE part of the example. I am unsure if that makes a difference. Let me know and I can implement this change as well.

The direct link between the two will most certainly be addressed in P&O :) For now, having the example applications separate won't be an issue for the normal user. I do suspect that more people will use the optimization example, but that's fine. Having the DSE example there will make all this functionality much more visible.

DominicDirkx commented 1 year ago

Does this make #82 outdated?

sbcowan commented 1 year ago

Okay I'll implement the changes you suggested.

Does this make https://github.com/tudat-team/tudat-space/pull/82 outdated?

The #82 pull request was already merged into master without explicitly merging the pull request, so yes that pull request is obsolete. This pull request is a follow-up (but will be changed by me again soon).

DominicDirkx commented 1 year ago

Thanks, clear, usually it updates the pull request automatically if it gets merged (even if this is done elsewhere)

DominicDirkx commented 1 year ago

Hi Sean, thanks! One thing, I may not have made my opinion very clear in my previous post, but I think it would be better to remove the DSE stuff from the optimization example, and have the DSE and optimization examples separate. Could you make this small modification? Other than that, this look really nice!

sbcowan commented 1 year ago

That is a mistake, I did remove that, but I guess saving in Jupyterlab somehow didn't go through. I will correct that now.

sbcowan commented 1 year ago

The mistake has been resolved.

DominicDirkx commented 1 year ago

And merged :)