Skip to content

ENH: haversine and exportElipses #247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 38 commits into from
Oct 6, 2022

Conversation

FranzYuri
Copy link
Contributor

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • [] Code docs are working correctly
  • Code base maintenance (refactoring, formatting, renaming):

    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally
  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

No way to export the ellipses of dispersion.

What is the new behavior?

Implementing exportElipsesToKML function.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Enter text here...

@FranzYuri
Copy link
Contributor Author

image
For some reason this "ellipse" is badly drawn even though I used several points to draw the polygon.
Also, I am not beeing able to draw ellipses inside other ellipses.

@Gui-FernandesBR Gui-FernandesBR changed the title haversine and exportElipses(incomplete) WIP: haversine and exportElipses Sep 27, 2022
@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes Refactor Outputs Dedicated to visualizations enhancements like prints and plots labels Sep 27, 2022
@Gui-FernandesBR Gui-FernandesBR linked an issue Sep 27, 2022 that may be closed by this pull request
latitude was X and longitude was Y. I fixed this
@brunosorban brunosorban changed the base branch from develop to enh/class_dispersion October 4, 2022 13:06
@Gui-FernandesBR
Copy link
Member

@brunosorban great work!

  • I have made some changes to tackle one of the bug, but another still remains: It's necessary to check the quadrant of direction before defining bearing angle value. I commented using TODOs, do you think you could take a look at that?
  • Is it possible to include different ellipses inside the same .kml file? That could be really helpful I guess.
  • Also, I just noted that I cannot import utilities inside Environment as the Environment is currently being called inside utilities. That's the reason the tests didn't pass. I have no clue of how to figure this out for now. Let's think about that for a while, could you help me on elaborating some hypothesis? Worst case scenario we move back functions from utilities to Environment and use @staticmethod decorators. But there should be an option!

@Gui-FernandesBR
Copy link
Member

@brunosorban
Copy link
Collaborator

  • The quadrant is actually not a problem since math.atan2() was used. This function already deals with it using numerator and denominator (sine and cosine) as input.
  • Different ellipses can now be added in the plots via type argument, where it can be all, where both impact and apogee ellipses are plotted, or user may choose only impact or apogee.
  • plotEllipses function was modified so it and exportEllipsesToKML use a common intermediate function called createEllipses. Since I was modifying the function, I also added 3 new optional arguments, so the user can choose: image perimeter size (before was fixed to 3000m), and change xlim and ylim.
  • User can also decide the color of the ellipses now. Selecting the number of ellipses (sigma 1 to sigma n) would be an improvement.
  • utilities.py seems to be a file that carries complementary scripts for further analyses. In this sense, I believe that the base classes should not depend on it. What I mean is, if a base class (in the case Environment.py) needs a function that is in utilities.py, this function should actually be on the base class.

@Gui-FernandesBR
Copy link
Member

  • The quadrant is actually not a problem since math.atan2() was used. This function already deals with it using numerator and denominator (sine and cosine) as input.
  • Different ellipses can now be added in the plots via type argument, where it can be all, where both impact and apogee ellipses are plotted, or user may choose only impact or apogee.
  • plotEllipses function was modified so it and exportEllipsesToKML use a common intermediate function called createEllipses. Since I was modifying the function, I also added 3 new optional arguments, so the user can choose: image perimeter size (before was fixed to 3000m), and change xlim and ylim.
  • User can also decide the color of the ellipses now. Selecting the number of ellipses (sigma 1 to sigma n) would be an improvement.
  • utilities.py seems to be a file that carries complementary scripts for further analyses. In this sense, I believe that the base classes should not depend on it. What I mean is, if a base class (in the case Environment.py) needs a function that is in utilities.py, this function should actually be on the base class.

Great work @brunosorban !! I will test the solution locally and then I can add my review.

Regarding the last point, well... you're right. Maybe we should define those utilities functions inside the base classes

However, the opposite of you sentence could also be true. We could use utilities.py to define abstract functiones such as haversine, which can be used in so many other places. Indeed, there's a lack of definition in the header of utilities.py and I'd like to raise the question of wether this is an "further analysis" of "auxiliary" class.

All inall, importing utilities after class Environment definition solved our problem until this point.

@Gui-FernandesBR Gui-FernandesBR changed the title WIP: haversine and exportElipses ENH: haversine and exportElipses Oct 6, 2022
@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review October 6, 2022 00:52
@Gui-FernandesBR Gui-FernandesBR requested review from Gui-FernandesBR and removed request for giovaniceotto October 6, 2022 00:52
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @brunosorban , thank you for such contribution, code is working properly and we are ready to go. I'm merging it into Dispersion Class

@Gui-FernandesBR Gui-FernandesBR merged commit 7d85a74 into enh/class_dispersion Oct 6, 2022
@Gui-FernandesBR Gui-FernandesBR deleted the enh/dispersion_ellipses branch November 3, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes Outputs Dedicated to visualizations enhancements like prints and plots Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export Dispersion Ellipses to .kml file
4 participants