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

Skydev issues110 77 #137

Merged
merged 15 commits into from
Sep 23, 2024
Merged

Skydev issues110 77 #137

merged 15 commits into from
Sep 23, 2024

Conversation

ymamay
Copy link
Contributor

@ymamay ymamay commented Aug 16, 2024

PR to merge skydev_issues110_77 into main. I updated and re-organized the sky header keywords and comments. Still needs more testing and there are four more skymodel header keywords that need to be added (see comments in skymodel_pars_header to add moon ra, moon dec, moon phase, and moon illumination). Also should update how the drpall grabs these keywords (note the shadow_height keywords are now sh_hght, and several of the {telescope} is removed). Closes #110.

@ymamay
Copy link
Contributor Author

ymamay commented Aug 16, 2024

Oh also I tried to format the header values so they would have a limited number of decimal points. In doing so I think the values are now strings, not sure if this is important for future functions (like building the drpall file)

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

Seems reasonable so far. Do all these header keywords have proper comments?

Comment on lines 237 to 238
#if telescope not in {"SKYE", "SKYW", "SCI", "SPEC"}:
# raise ValueError(f"invalid value for 'telescope' parameter: '{telescope}', valid values are 'SKYE', 'SKYW', 'SCI', or 'SPEC'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't need this anymore, then you can remove it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, guess I forgot that one, thanks for catching it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't see it anymore in my current version, so I think I did remove it.

python/lvmdrp/core/sky.py Outdated Show resolved Hide resolved
python/lvmdrp/core/sky.py Outdated Show resolved Hide resolved
python/lvmdrp/core/sky.py Outdated Show resolved Hide resolved
python/lvmdrp/functions/skyMethod.py Outdated Show resolved Hide resolved
@ymamay
Copy link
Contributor Author

ymamay commented Sep 12, 2024

@ajmejia @havok2063 I think this is ready to be merged now. Ran on a subset of the refdata and the drpall file looked ok

@havok2063
Copy link
Collaborator

Some of the CI isn't passing. See the failing checks. Can you merge the latest master into this branch? This should clear up some of them. It looks like there may be other linting issues that need to be resolved.

@havok2063
Copy link
Collaborator

Looks good to me!

@ajmejia
Copy link
Contributor

ajmejia commented Sep 23, 2024

Looks good! Thanks!

@ajmejia ajmejia merged commit a6bad3a into master Sep 23, 2024
8 checks passed
@ajmejia ajmejia deleted the skydev_issues110_77 branch September 23, 2024 13:53
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.

shadow height header keyword wrong
3 participants