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.

Sign up for GitHub

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

Jump to bottom

Add focus_z to properties. #58

Merged
merged 5 commits into from Jun 23, 2022
Merged

Add focus_z to properties. #58

merged 5 commits into from Jun 23, 2022

Conversation

jbkalmbach
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jun 13, 2022 •
edited

Codecov Report

Merging #58 (c555383) into main (c555383) will not change coverage.
The diff coverage is n/a.

❗ Current head c555383 differs from pull request most recent head f26ae4d. Consider uploading reports for the commit f26ae4d to get more accurate results

@@           Coverage Diff           @@
##             main      #58   +/-   ##
=======================================
  Coverage   78.44%   78.44%           
=======================================
  Files          36       36           
  Lines        3085     3085           
  Branches      549      549           
=======================================
  Hits         2420     2420           
  Misses        543      543           
  Partials      122      122

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c555383...f26ae4d. Read the comment docs.

@jbkalmbach jbkalmbach force-pushed the tickets/DM-35186 branch 3 times, most recently from 5512965 to e3c9b77 Compare June 14, 2022 18:51
@jbkalmbach
Copy link
Member Author

I added DECAM after checking with Aaron Roodman. I wasn't 100% sure on any of the others in here so I decided to not touch them for now. Will add a PR in obs_lsst that adds tests for this header keyword in the LATISS translator there.

@jbkalmbach jbkalmbach requested a review from timj June 14, 2022 18:55
@timj
Copy link
Member

timj commented Jun 14, 2022

@PaulPrice should we use FOC-VAL for HSC translator?

I'm not sure what MegaPrime uses because its TELFOCUS seems to be an integer.

@PaulPrice
Copy link
Contributor

FOC-VAL seems reasonable to me.
I could believe that MegaCam is quantising their values, or measuring in discrete steps.

timj
timj requested changes Jun 14, 2022
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Thanks. I think the default implementation needs to be moved to MetadataTranslator.

Please also add a HSC translation.

python/astro_metadata_translator/translators/decam.py Show resolved Hide resolved
python/astro_metadata_translator/translators/fits.py Outdated Show resolved Hide resolved
python/astro_metadata_translator/translators/helpers.py Outdated Show resolved Hide resolved
tests/test_decam.py Show resolved Hide resolved
python/astro_metadata_translator/translators/helpers.py Outdated Show resolved Hide resolved
@jbkalmbach jbkalmbach force-pushed the tickets/DM-35186 branch 3 times, most recently from c00f67a to 8397839 Compare June 14, 2022 22:36
@jbkalmbach jbkalmbach requested a review from timj June 15, 2022 18:28
@jbkalmbach
Copy link
Member Author

Thanks for the help. I've moved the defaults over to MetadataTranslator and added the HSC translation and tests for HSC and MegaCam.

timj
timj approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

A couple of minor comments. Sorry I took so long to review again, my Jira dashboard didn't show me as a reviewer for DM-35186.

python/astro_metadata_translator/translator.py Outdated Show resolved Hide resolved
python/astro_metadata_translator/translators/decam.py Outdated Show resolved Hide resolved
tests/test_basics.py Outdated Show resolved Hide resolved
@jbkalmbach jbkalmbach merged commit 83a23ae into main Jun 23, 2022
@jbkalmbach jbkalmbach deleted the tickets/DM-35186 branch June 23, 2022 21:42
@jbkalmbach
Copy link
Member Author

Thanks. Made requested changes and merged.

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

@timj timj timj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jbkalmbach @timj @PaulPrice