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

Exclude outdated solar system objects #1880

Merged
merged 11 commits into from
Sep 23, 2021
Merged

Exclude outdated solar system objects #1880

merged 11 commits into from
Sep 23, 2021

Conversation

alex-w
Copy link
Member

@alex-w alex-w commented Sep 11, 2021

Description

Excluding the Solar system objects with outdated data from ephemeris, positions and phenomena computations

Fixes #1564

Screenshots (if appropriate):

v0.21.1:
stellarium-006

v0.21.1-122fe98:
stellarium-007

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?

Load 1000 comets data file.
Use AstroCalc/Phenomena (AstroCalc/Positions or AstroCalc/Ephemeris also) to search for phenomena around Jupiter's moons, e.g. June to September 2021.

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

@github-actions
Copy link

github-actions bot commented Sep 11, 2021

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files

@alex-w alex-w added this to To do in Astronomical calculations (AstroCalc) via automation Sep 11, 2021
@alex-w alex-w added this to To do in Solar System via automation Sep 11, 2021
@alex-w alex-w moved this from To do to In progress in Astronomical calculations (AstroCalc) Sep 11, 2021
@alex-w alex-w moved this from To do to In progress in Solar System Sep 11, 2021
@alex-w alex-w added bug Something likely wrong in the code enhancement Improve existing functionality labels Sep 11, 2021
@alex-w alex-w added this to the 0.21.2 milestone Sep 11, 2021
@github-actions
Copy link

Hello @alex-w! Thank you for this enhancement.

@axd1967 axd1967 mentioned this pull request Sep 16, 2021
@axd1967
Copy link
Contributor

axd1967 commented Sep 16, 2021

The user would benefit from being informed about the problem, as it is not obvious that ephemeris data is outdated (as we can judge from several issues that are reported), especially for comets/asteroids, as these require the user to open the Solar System Editor, which is a tad hairy to manipulate. All too often the users assume that the data is always up-to-date, which is obviously NOT the case. So until an automatic update is possible, the user should be aware of the problem.

at least emit a QWarning() in the log file for each object that is outdated.
It looks like SolarSystem::loadPlanets() could be a good location for such a warning:

const double orbitGoodDays=pd.value(secname+"/orbit_good", parent->englishName!="Sun" ? 0 : 1000).toDouble(); // "Moons" have permanently good orbits.

nicer but much more expensive is to (also) highlight outdated objected in the tabular GUIs (eg grey/italic?)

src/core/modules/Planet.hpp Outdated Show resolved Hide resolved
src/core/modules/Planet.hpp Outdated Show resolved Hide resolved
@@ -4473,6 +4480,19 @@ QMap<double, double> AstroCalcDialog::findClosestApproach(PlanetP& object1, Stel
QMap<double, double> separations;
QPair<double, double> extremum;

if (object2->getType()=="Planet")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do you also see the inefficiency?
For a simple date validity test you set the whole program state twice.

Please do the following: extend
bool Planet::hasValidPositionalData(const double jde)
{
if (pType<isObserver)
return true;
else if (orbitPtr && pType>=isArtificial)
return static_cast<KeplerOrbit*>(orbitPtr)->objectDateValid(jde);
else
return false;
}

And then you can simply test here:

if ( (!planet->hasValidPositionalData(stopJD)) || (!planet->hasValidPositionalData(startJD)) )
return separations; // empty result.

And probably this is not even what we need here? Imagine some object with orbit_good=100 (days)

You make an ephemeris for 250 days which enclose the orbit's validity range. The object would be excluded with the current solution! Maybe add a test for (startJD+stopJD)/2? Or sample every 50 days between startJD and stopJD? Or is there a limit or typical value for (stopJD-startJD)? If this is always something like 10 days, you could exclude the object if both dates are invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added JDE as parameter to reduce number of calls, but for second proposal I think it should be postponed to the future releases, because this subsystem should be refactored after pencil and paper works.

Copy link
Member

Choose a reason for hiding this comment

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

OK. The inefficiency is solved.
The next question is just what this function should do. Is this logic correct? Whenever at least one limit of the search interval is outside the validity range for the planet, the planet is discarded. I see a danger of another error. If you need to merge this now, leave a FIXME with this question for later, and maybe somebody reports an error. Else please don't merge, and let's solve this with better tests for the overlapping cases. (We may need to add some really simple date validity queries to the Planet.)

Copy link
Contributor

Choose a reason for hiding this comment

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

please add the QWarning() logging, or some other warning mechanism. the QWarning() is not much asked, and will help if a user opens an issue; the log will explain what happened.

thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

The next question is just what this function should do. Is this logic correct?

Yes, but I'll change it as first iteration of excluding the data.

@todo

This comment has been minimized.

@todo

This comment has been minimized.

@lgtm-com
Copy link

lgtm-com bot commented Sep 22, 2021

This pull request introduces 2 alerts when merging 927a249 into 96c0334 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@gzotti
Copy link
Member

gzotti commented Sep 23, 2021

I looks much faster, but I may have found another bug.
stellarium-007
Eclipse between Jupiter and Neptune? With more than 3° separation this cannot be an occultation. Maybe Jupiter in transit as seen from Neptune? (Goto Neptune, but seen no transit).
This is a separate issue though.
Also, I think shadow transit checks can safely be reduced to the respective planet's moons, not things like comet and Jupiter. Yet another separate issue.

@alex-w
Copy link
Member Author

alex-w commented Sep 23, 2021

I found today this issue too and this is puzzled me...

@gzotti gzotti self-requested a review September 23, 2021 10:45
Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

I think the purpose of the branch is fulfilled. Newly found old weirdnesses which we now have found should be fixed in other issues/PRs. (0.21.1 finds the same things, so that's not a problem of this PR.)

@alex-w
Copy link
Member Author

alex-w commented Sep 23, 2021

I'm work on fix of founded issue - just testing the fix right now in this branch

@alex-w
Copy link
Member Author

alex-w commented Sep 23, 2021

OK, please check the fix

@gzotti
Copy link
Member

gzotti commented Sep 23, 2021

OK. No more eclipses Neptune/Jupiter.
However, running 0.21.1 and this branch side-by-side, the shadow transit events at Jupiter differ: One finds an event for Io, the other finds Europe a few hours later, or vice versa. Both are correct in what they are finding, but just seem not to find all events. The search intervals in the phenomena tab overlap but are not identical, if that matters. (It should not!)

@alex-w alex-w merged commit b87b419 into master Sep 23, 2021
@alex-w alex-w deleted the exclude-outdated-comets branch September 23, 2021 11:20
Solar System automation moved this from In progress to Done Sep 23, 2021
Astronomical calculations (AstroCalc) automation moved this from In progress to Done Sep 23, 2021
@alex-w
Copy link
Member Author

alex-w commented Sep 28, 2021

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something likely wrong in the code enhancement Improve existing functionality
Development

Successfully merging this pull request may close these issues.

Exclude outdated comet data in ephemeris computations
3 participants