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

feat: DAMD-137 (proper motion and parallax in pfsDesign and pfsConfig) #117

Merged
merged 4 commits into from Jul 25, 2023

Conversation

monodera
Copy link
Contributor

  • Add epoch, pmRa, pmDec, and parallax for targets in the DESIGN table of pfsDesign and pfsConfig with the same formats as those in the GUIDESTAR table.
  • Modify datamodel.txt and tests_PfsConfig.py accordingly.

Copy link
Member

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

This looks reasonable, but there need to be some changes before merging. In particular, test_PfsFluxReference.py fails due to this change, and I'm sure there must be similar failures in drp_stella.

datamodel.txt Outdated
@@ -385,6 +385,10 @@ The DESIGN table lists for each object:
targetType 32-bit int (enumerated type: e.g. SCIENCE,SKY,FLUXSTD)
fiberStatus 32-bit int (enumerated type: e.g. GOOD,BROKENFIBER,BLOCKED,BLACKSPOT)
pfiNominal pair of 32-bit floats (millimeters on the PFI)
epoch 7-digit string
Copy link
Member

Choose a reason for hiding this comment

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

What is the meaning of the "7-digit string"? Can you give an example? Wouldn't a floating-point MJD or similar be faster, since strings require parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from the explanation in the guide star table which was written by someone before. It's something like "J2000.0".

Copy link
Member

Choose a reason for hiding this comment

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

I think "7-character string" would be more accurate. "Digit" implies solely numbers.

@@ -176,6 +186,10 @@ class PfsDesign:
"catId": "J",
"objId": "K",
"targetType": "J",
"epoch": "A",
"pmRa": "D",
"pmDec": "D",
Copy link
Member

Choose a reason for hiding this comment

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

You have previously specified these as single-precision floats, but are saving them as double-precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to "E".

I actually copied from guideStars.py and it also has apparently inconsistent definitions (at lines around 138).

            Column("pmRa", "D", array=self.pmRa, unit='mas/year'),
            Column("pmDec", "D", array=self.pmDec, unit='mas/year'),
            Column("parallax", "E", array=self.parallax, unit='mas'),

Copy link
Member

Choose a reason for hiding this comment

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

Would you fix those (in a separate commit) while you're in there, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -998,6 +1027,10 @@ class PfsConfig(PfsDesign):
"catId": "J",
"objId": "K",
"targetType": "J",
"epoch": "A",
"pmRa": "D",
"pmDec": "D",
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@monodera
Copy link
Contributor Author

I fixed test_PfsFluxReference.py, but not those for drp_stella.

@PaulPrice
Copy link
Member

They will need to be fixed before merging.

@monodera
Copy link
Contributor Author

Could you please let me know which files should I look at in drp_stella? (I'm a bit overwhelmed by the large codebase...)

@PaulPrice
Copy link
Member

The best way would be to run the tests (setup -jr . and then scons) and fix anything that fails. Alternatively, you could grep for the PfsConfig constructor.

@monodera
Copy link
Contributor Author

I did the test and made modifications accordingly. The corresponding PRs have been made in drp_stella.

- Add `epoch`, `pmRa`, `pmDec`, and `parallax` for targets in the DESIGN table of pfsDesign and pfsConfig with the same formats as those in the GUIDESTAR table.
- Modify `datamodel.txt` and `tests_PfsConfig.py` accordingly.
@alefur alefur merged commit 185a570 into master Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants