Skip to content

Prefer numeric degree RA/Dec fields in VOTable parsing#383

Open
tboch wants to merge 1 commit into
developfrom
fix-votable-ra-dec-field-scoring
Open

Prefer numeric degree RA/Dec fields in VOTable parsing#383
tboch wants to merge 1 commit into
developfrom
fix-votable-ra-dec-field-scoring

Conversation

@tboch

@tboch tboch commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

@tboch
tboch requested a review from bmatthieu3 June 25, 2026 10:50
Comment thread src/js/ProgressiveCat.js
return fields;
}

function isUCDField(field, ucdField, oldUcdField) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems to be the same code as in Catalog. Maybe put those definition in Catalog, export them so that they can be imported in ProgressiveCat as well.

Comment thread src/js/Catalog.js
var isDouble = datatype === "double";
var isDeg = unit === "deg";

if (datatype && datatype !== "char" && datatype !== "unicodechar") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this tests seems to prefer non char. the idea is to prefer numeric values I think (int, double, float, complex, boolean, short). Do we consider integer/complex/boolean to be legit for a position so that we increase the score comparatively to column position that would be in sexagesimal (char) ? I would just add a bonus for double/float.

Comment thread src/js/Catalog.js
if (isDeg) {
score += 10;
}
if (isDouble && isDeg) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this case ? Having isDouble + isDeg is already better than just isDouble or just isDeg (the two adds up to 30 but having one of them adds up to 10 or 20).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants