Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error in Orbit.from_sbdb when more than one orbit is returned #609

Closed
shreyasbapat opened this issue Mar 18, 2019 · 10 comments

Comments

@shreyasbapat
Copy link
Member

commented Mar 18, 2019

馃悶 Problem

When Orbit.from_sbdb is called with names which are too common, like "Halley" or "*alley", it returns a keyerror. Raise a suitable warning in those cases.

Code to reproduce error:

Orbit.from_sbdb("Halley")

馃枼 Please paste the output of following commands

  • conda info -a (only if you have conda)
  • conda list (only if you have conda)
  • pip freeze
# Paste your output here:
  • python -c "import poliastro.testing; poliastro.testing.test()"
# Paste your output here:

馃幆 Goal

Related to #534
To make from_sbdb more user friendly.

馃挕 Possible solutions

馃搵 Steps to solve the problem

  • Comment below about what you've started working on.
  • Add, commit, push your changes
  • Submit a pull request and add this in comments - Addresses #<put issue number here>
  • Ask for a review in comments section of pull request
  • Celebrate your contribution to this project 馃帀
@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Rather than a warning, I think we should raise an error, in the same way that we did with the old API:

Screenshot_2019-03-18 Using NEOS package 鈥 poliastro 0 12 0 documentation

@Juanlu001 Juanlu001 changed the title Add warnings to Orbit.from_sbdb when more than one orbit is returned Raise error in Orbit.from_sbdb when more than one orbit is returned Mar 26, 2019

@Ivan9528

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

What about raising an error a bit more elaborated/understandable for the user about the different bodies found and then giving the user the chance to chose one of them by entering the complete name or the code number associated with it?

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@Ivan9528 thanks for chiming in! The problem with that is that it requires interactivity, and in a script that runs unattended it can block the program. #611 is unfinished, so perhaps you want to take care of this one?

@michiboo

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hi! is this issue still unsolved? if not I will try tackle it!

@Ivan9528

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Hi @michiboo!! It is already solved actually, I just have to make the pull request but the coding is done, I will do it tonight if time allows me to :D Thanks!!

@Ivan9528

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@michiboo I am sorry, I am not working on this issue but on #611. Anyway, @Juanlu001, please confirm that this issue is unsolved because I have run the code I have got a value error instead of a key one which I presume it was the purpose of the issue...

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

There was an effort to tackle this in #611 . But the PR author hasn't replied since.

I have run the code I have got a value error instead of a key one which I presume it was the purpose of the issue...

Did you run it after making changes for #611 ? Because #611 is solution for this issue :)

@Ivan9528

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Yes, I run it and it raises the error as the previous coder already did but changing slightly the coding. I am going to try to make the pull request so that you any can review it :D

@jorgepiloto

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Great @Ivan9528! 馃槃

@Juanlu001 Juanlu001 added this to Backlog in poliastro/poliastro Apr 26, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Closed in #651.

@Juanlu001 Juanlu001 closed this Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can鈥檛 perform that action at this time.