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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added geographic coordinates of center line for total and annular solar eclipse #1116

Merged
merged 12 commits into from
Jun 15, 2020
Merged

Added geographic coordinates of center line for total and annular solar eclipse #1116

merged 12 commits into from
Jun 15, 2020

Conversation

worachate001
Copy link
Member

Description

This added method for calculation of geographic coordinates where we can see total or annular eclipse. Predicted coordinates may not be very accurate because changing value of deltaT (TT-UT1).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

Test Configuration:

  • Operating system: Windows 10

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Show magnitude of lunar eclipse
fixed jumping Moon
…ar eclipse

Calculation of geographic coordinates where we can see total or annular eclipse. Predicted coordinates may not be very accurate because changing value of deltaT (TT-UT1).
@worachate001
Copy link
Member Author

It looks like I should use the command git rebase master before sending a pull request, right?

@alex-w
Copy link
Member

alex-w commented Jun 8, 2020

Please see lines 901-916, which already using for solar eclipses. I think your code can be moved into separate private method with return Vec3d(lat, lon, dratio) value and used within block if (onEarth && obj==ssystem->getMoon()) to reduce calculations and avoid possible slowing work of the Stellarium (on powerless PC), when the Sun is selected.

@alex-w
Copy link
Member

alex-w commented Jun 8, 2020

It looks like I should use the command git rebase master before sending a pull request, right?

It's not needed in this case

@gzotti
Copy link
Member

gzotti commented Jun 8, 2020

Thank you for writing these functions. But please take into account that Stellarium still has no complete aberration correction. I did some trickery to match local contact times in the simulation, but would have solved aberration before tackling these high-accuracy problems. In any case make sure to document your code (have not checked now), so that it can be understood and maintained.

On the other hand, the built-in default DeltaT should be perfectly usable. A perfect solution should agree to the second with some expert reference, of course, and divergences can be helpful to identify errors still hidden somewhere.

@alex-w alex-w requested a review from gzotti June 10, 2020 14:10
@alex-w
Copy link
Member

alex-w commented Jun 12, 2020

@worachate001 I've reviewed your code again and I have a question: are you wanting to show an eclipse info since the moment of eclipse startup anywhere on Earth or for current location? If you planned to show an eclipse info not dependent on the current location then my proposal about call calculations within the block if (onEarth && obj==ssystem->getMoon()) was wrong.

@worachate001
Copy link
Member Author

@worachate001 I've reviewed your code again and I have a question: are you wanting to show an eclipse info since the moment of eclipse startup anywhere on Earth or for current location? If you planned to show an eclipse info not dependent on the current location then my proposal about call calculations within the block if (onEarth && obj==ssystem->getMoon()) was wrong.

Yes. My intention at first is not depend on location. If you think moving out of that block is okay (not too much calculations going on when selecting the Sun), I'm happy to move them again.

@alex-w
Copy link
Member

alex-w commented Jun 12, 2020

Oops... sorry for my bad suggestion, please move them again.

I have a new one question now. You are creating a special class for calculating the central point of the eclipse, but some part of calculation is outside the method of the new class. What reasons did you not move all required code into one method?

@worachate001
Copy link
Member Author

I see what you mean. I will do some testing and will send the updated code again when ready. Thanks for suggestions.

@worachate001
Copy link
Member Author

I carefully compared Stellarium's results with some solar eclipses in the past, present, and future (in NASA web site - https://eclipse.gsfc.nasa.gov/SEcat5/SEcatalog.html) by using the same DeltaT for each dates.

Despite incomplete implementation of aberration, the results are very close except a few minutes near the beginning and end of central eclipse (when the Sun is very close to the horizon) and eclipses that occur near the poles.

Main things that affect accuracy of center line prediction:

  • different values of true vs predicted DeltaT
  • positions of the Sun and Moon
  • aberration, nutation and obliquity of ecliptic - they have small values but can cause large shift of shadow on Earth

My observation:

  • gradually disabled of nutation before the year 1500 in Stellarium makes noticeable differences in position of the Sun and Moon compared with JPL HORIZONS, fully activate nutation may be better
  • I will try to further investigate and find ways to improve Sun & Moon positions

@gzotti
Copy link
Member

gzotti commented Jun 13, 2020

Hi! Thank you for these observations. TSE are certainly cases where computational models and their correct application, or errors of doing so, are most obvious. I am just a bit conservative w.r.t. effects of nutation as I could not find indications of a time frame of validity for the model. If the model is safe for +/- 2000.0 or even 10.000 years or more, we can change that of course, but I'd like to have some confirmation on this.

A few weeks ago somebody remarked our nutation matrix is wrong. See Planet::computeTransMatrix(). I followed one paper, that reporter mentioned a different source. Without a critical case, I cannot decide it, given the other small errors. You could play with the sign of the deltaPsi rotation to see if one is better.

See also StelObserver::getTopographicOffsetFromCenter(). Any error here will also shift things.

See also SolarSystem::computePositions() about the TSE hack which hides the aberration error for TSE. Another critical case to test is Venus occultation on June 19th, 2020.

@alex-w
Copy link
Member

alex-w commented Jun 13, 2020

I checked 2 solar eclipses for this year and found small difference for coordinates of central point, but not for time and magnitude.

@alex-w
Copy link
Member

alex-w commented Jun 13, 2020

@gzotti please review the patch

src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
.arg(QChar(0x00B0))
.arg(pos[1], 5, 'f', 4)
.arg(QChar(0x00B0));
oss << "<br/>";
Copy link
Member

Choose a reason for hiding this comment

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

You could add great-circle distance to observer. I thought we had it implemented already, but now I see only StelLocation::distanceDegrees(). We could add double StelLocation::distanceKm(long1, lat1, long2, lat2), following Meeus, Astron. Algorithms (2nd ed 1998) ch.11, p.85. If you don't have the book, I can do it.

Copy link
Member

Choose a reason for hiding this comment

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

I just added such a function to StelLocation. It may even be interesting to say: "Shadow centerpoint is 45km towards azimuth 63°". The azimuth function can be taken from ArchaeoLines::getAzimuthForLocation().

Copy link
Member

Choose a reason for hiding this comment

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

@gzotti ArchaeoLines may be not loaded ;) Maybe move getAzimuthForLocation() into StelLocation class?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant not "use Archaeoline's function", but "grab code". Indeed, when used elsewhere, this function should be moved, e.g. to StelLocation or StelUtils.
@worachate001 I am sorry I also implemented the missing bit, now we have a conflict. My implementation is more general, so I'd prefer to keep this. You can move the static functions Archaeolines::getAzimuthForLocation() as Alexander suggested to StelLocation, adapt ArchaeoLines accordingly, and maybe add the non-static function StelLocation::AzimuthForLocation(). I think these make interesting additions!

src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
improved and compute great-circle distance from observer to point of central eclipse as suggested by gzotti
@worachate001
Copy link
Member Author

Thanks for suggestions. I'm not sure that the "Resolve conversation" button is for me or for you. I think I have done all of your suggestions. Feel free to suggest more, Visual C++ Programming is new for me.

@gzotti gzotti merged commit b2b60dc into Stellarium:master Jun 15, 2020
@gzotti
Copy link
Member

gzotti commented Jun 15, 2020

@worachate001 Great new feature, many Thanks!

@gzotti gzotti added the enhancement Improve existing functionality label Jun 15, 2020
@gzotti gzotti added this to the 0.20.2 milestone Jun 15, 2020
@worachate001 worachate001 deleted the solareclipse branch June 16, 2020 01:34
@worachate001 worachate001 restored the solareclipse branch June 16, 2020 01:34
@worachate001 worachate001 deleted the solareclipse branch June 16, 2020 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality
Development

Successfully merging this pull request may close these issues.

3 participants