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

Cannot find 1P/Halley in historical apparitions #3349

Closed
gzotti opened this issue Aug 3, 2023 · 21 comments · Fixed by #3373
Closed

Cannot find 1P/Halley in historical apparitions #3349

gzotti opened this issue Aug 3, 2023 · 21 comments · Fixed by #3373
Assignees
Labels
bug Something likely wrong in the code data Missing/bad/outdated data, but no code error importance: high Obvious error, very annoying, but no crash
Milestone

Comments

@gzotti
Copy link
Member

gzotti commented Aug 3, 2023

After the big renaming effort, the F3 search panel no longer can clearly indentify a comet in several apparitions.

Expected Behaviour

F3 search: "Halley" should list all known apparitions

Actual Behaviour

I can only find one entry for 1P/Halley which resolves to 989 N1, and another for P/Halley (1222) which does not load.

Steps to reproduce

Load ssystem_1000comets.ini, then search for Halley.

System

  • Stellarium version: 23.2
  • Operating system: Win11 (hopefully irrelevant)
  • Graphics Card: Geforce (irrelevant)

Logfile

If possible, attach the logfile log.txt from your user data directory. Look into the Guide for its location.
The logfile indicates some message around a "stupid general group" which IMHO should not exist and indicates a bug.
Logfile of a previous run showed problems around a regular expression ('HALLEY(1456)'), but I cannot reproduce that now. I searched for "Halley", "Halley(1456)" and "Halley (1456)".

log.txt

@gzotti gzotti added the importance: high Obvious error, very annoying, but no crash label Aug 3, 2023
@gzotti gzotti added this to the 23.3 milestone Aug 3, 2023
@gzotti gzotti added this to Needs triage in Solar System via automation Aug 3, 2023
@alex-w alex-w self-assigned this Aug 3, 2023
@alex-w
Copy link
Member

alex-w commented Aug 3, 2023

@gzotti did you used 1000 comets from master?

@gzotti
Copy link
Member Author

gzotti commented Aug 3, 2023

No, 23.2 with the data file from its own directory. Did you change something later? (I don't usually build the SSE plugin, so just used the regular program.)

@alex-w
Copy link
Member

alex-w commented Aug 3, 2023

Please attach data file

@alex-w alex-w added the bug Something likely wrong in the code label Aug 3, 2023
@gzotti
Copy link
Member Author

gzotti commented Aug 3, 2023

You can use files from master or from 23.2. Result is the same. Does it work (does it list and let you you select any of the expected Halley entries) on Linux?
The 1222 entry was a stored query, I reset them, now it's gone. The issue persists in master.

@alex-w
Copy link
Member

alex-w commented Aug 14, 2023

@gzotti I can't reproduce the "group" issue on Windows too - what you did doing?

@gzotti
Copy link
Member Author

gzotti commented Aug 14, 2023

  • Delete USERDIR/data/ssystem_minor.ini
  • start Master
  • Reset last search items
  • Search Halley

Result: This only finds P/Halley from 989. When I set date to e.g. 1456, Halley (in its apparition of 1456) is not displayed.

log.txt

Line 3544:

Checking for stupid General group.

@alex-w
Copy link
Member

alex-w commented Aug 15, 2023

  • Delete USERDIR/data/ssystem_minor.ini
  • start Master
  • Reset last search items
  • Search Halley

Result: This only finds P/Halley from 989. When I set date to e.g. 1456, Halley (in its apparition of 1456) is not displayed.

Yes, I can reproduce this issue, and it's expectable due to the current implementation of search/matching the data in Search Tool.

log.txt

Line 3544:

Checking for stupid General group.

I don't see these warnings in my log

@gzotti
Copy link
Member Author

gzotti commented Aug 15, 2023

Then the search/match tool must be fixed accordingly. But I am afraid there is even more. In addition to not being found via search, I simply cannot find the comet visualized in 1456, when it was reportedly observed. The 1457 comets appear as expected.

@gzotti
Copy link
Member Author

gzotti commented Aug 15, 2023

How was this ever planned to work?
englishName is always 1P/Halley. Making this unique (renaming to a wrong name) appears to solve the problem, i.e., show the comet, but of course runs against the idea of the recent changes.
Or, better, overriding Planet::getID() and using Comet::getID(). getID() must be a unique string.

@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

@gzotti please check the Search Tool/Lists/Comets - probably the issue is deeper that we think :(

@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

How was this ever planned to work? englishName is always 1P/Halley. Making this unique (renaming to a wrong name) appears to solve the problem, i.e., show the comet, but of course runs against the idea of the recent changes. Or, better, overriding Planet::getID() and using Comet::getID(). getID() must be a unique string.

Maybe. The solution looks acceptable. Possible unique ID for comets with periodic designation is - "designation [perihelion year]" (e.g. "1P/Halley [1986]").

@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

OK, Stellarium cannot load multiple celestial bodies with same names, so, we have possible 2 solutions:
a) using unique names for comets with periodic designations (example: 1P/Halley [1986], where 1P/Halley is periodic designation and [1986] is year of reaching the perihelion)
b) modify the code to allow loading celestial bodies with identical names and add the hook to generate the unique proper names for comets with periodic designations

Both ways have a pluses and minuses and I prefer the way a) right now - we need update data file only (with revision the data)

@alex-w alex-w added the data Missing/bad/outdated data, but no code error label Aug 16, 2023
@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

No need to update the data file! We have everything required to form a unique string for getID(). "<englishName> (<date_code>)". And all relevant places where englishName was tested should test getID() instead. If the search dialog receives a list of getID(), there should then be numerous entries for all entries Halley, Pons, Biela, Levy, ...

@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

No need to update the data file! We have everything required to form a unique string for getID(). "<englishName> (<date_code>)". And all relevant places where englishName was tested should test getUID() instead. If the search dialog receives a list of getID(), there should then be numerous entries for all entries Halley, Pons, Biela, Levy, ...

You're not right - please see filling secNameMap and depLevelMap within method SolarSystem::loadPlanets() - if you have 10 items for one comet, then you will get 10 records for the latest one.

@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

Please see first part of changes in branch fix/3349

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

No need to update the data file! We have everything required to form a unique string for getID(). "<englishName> (<date_code>)". And all relevant places where englishName was tested should test getUID() instead. If the search dialog receives a list of getID(), there should then be numerous entries for all entries Halley, Pons, Biela, Levy, ...

You're not right - please see filling secNameMap and depLevelMap within method SolarSystem::loadPlanets() - if you have 10 items for one comet, then you will get 10 records for the latest one.

Indeed, here is a trap, and has always been there. Before the line
secNameMap[englishName] = secname;

we must test if secNameMap already contains such entry, and give a qWarning(). And in case of comets, where problems are now to be expected, you can just use "<englishName> (<date_code>)" as key into secNameMap. This obviously requires a valid date_code for all comet entries. If a comet entry has no date_code, give a qWarning() and ignore entry.

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

It appears to me that most entries "iau_designation" for minor planets and the two Interstellar objects should be "discovery_code". Of course, IAU acknowledges progress in knowledge, so that they would use the refined designation like "(number) name".

@alex-w alex-w moved this from Needs triage to In progress in Solar System Aug 16, 2023
@alex-w
Copy link
Member

alex-w commented Aug 16, 2023

No need to update the data file!

You’re wrong again, because many comets in ssystem_1000comets.ini file has joined designations as a name of the comet. We should split these data into separate parameters.

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

I am speaking of the default ssystem_minor.ini. The 1000comets.ini has, as acknowledged in its header, still the old names, apart from the Halley entries. Please don't edit that now, I have just removed 6500 lines of default entries from there. After merge of #3371 it appears useful to update this file according to the latest developments.

I will have a deeper look into this myself now and try to find a solution without even more extra data or variables. The object ID is still unused (or rather, equals englishName which is not unique here). Within the program, objects should be identified unambiguously with getID(), and to the user all labels can be shown in the Infostring, but it should be enough to use the combined name+date code.

Solar System automation moved this from In progress to Done Sep 11, 2023
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 11, 2023
@github-actions
Copy link

Hello @gzotti!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Sep 26, 2023
@github-actions
Copy link

Hello @gzotti!

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 data Missing/bad/outdated data, but no code error importance: high Obvious error, very annoying, but no crash
Projects
Solar System
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants