Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Keplerian orbits #279
base: master
Are you sure you want to change the base?
Keplerian orbits #279
Changes from 3 commits
ba1a82b
23d6a79
899bea8
1720587
43f468b
b32de43
77af67f
53146a0
1df4986
5170598
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes this sometimes -> the algorithm sometimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make
tolerance
into aconstexpr
variable here can't you?static constexpr tolerance = 1.0E-10;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I recommend wrapping the individual portions of your conditional expression in parens.
Watched a lot of people mistake the order of evaluation here too... Had a few bugs in prod because of it at work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the extra newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any value in taking
ecc
andv
as references instead of by value?At least on x86_64 linux, with clang,
sizeof(double)
is 8bytes, which is the same as a pointer.So on most platforms, when passing by value, the double should be passed via register (assuming few enough parameters). On platforms where there isnt enough space to pass by register, it'll implicitly pass by reference / pointer regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these math functions look like they are
noexcept
right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am a noob at c++ but in (76-77) of kerpler.cpp double df_E = 1.0 - eccentricity * cos(E); // if eccentricity happens to be 1, a div by 0 , since c++ do-while loops execute at least once, idk if that causes issues in c++ / this specific implementation (ie it may check if eccentricity is 1 and use some other code) but every language i actually have skills in throws a exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that floating point exceptions are not the same as C++ exceptions. I think if it does a divide by zero, the program will simply crash
regardless, if crashing isn't the right answer, then what should the function do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More accurately, on posix platforms it'll receive a signal that interrupts program flow and calls a signal handler (which we have no handler registered, so it'll default to crashing)
on windows it'll likely result in a structured exception handler exception being thrown (which is not the same thing as a C++ exception, and is handled completely differently, just like signal handling is done totally differently)
Regardless, throwing a c++ exception isnt the right answer, we need to have a solid determination of what we want the program to do if div by zero happens in this function.
Crashing is one choice, or we can return some kind of error code, but we want to document what the behaivor is going to be and then stick to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A floating point math error isn't necessarily an unrecoverable error for the end application, so I don't know if crashing is a good default choice choice for handling them.
I'm not well versed in c++ error handling though, so I don't know what the right way to handle floating point errors would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recommendation is to add
noexcept
unless someone provides a signal-handler -> exception translator, which very well might happen.Until then,
noexcept
can result in better code-gen in many situations, helps analysis tools, and it can always be removed later.