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 astropy.units support to config layer. #1311

Merged
merged 15 commits into from
Sep 18, 2024
Merged

Add astropy.units support to config layer. #1311

merged 15 commits into from
Sep 18, 2024

Conversation

jmeyers314
Copy link
Member

Starting to add astropy Quantity/Unit support to GalSim config layer... WIP, but could be worth a quick look at what's here so far.

@jmeyers314 jmeyers314 marked this pull request as ready for review September 16, 2024 22:16
@jmeyers314
Copy link
Member Author

I think this is ready now. I added Quantity and Unit as config-layer value types. These can be instantiated as described in #1194 . I then added support for a number of concrete config classes, including:

  • Airy, Kolmogorov, VonKarman, OpticalPSF for wavelengths, Fried parameters, telescope diameters.
  • Bandpass units
  • SED units and flux densities
  • DCR conditions (pressure/temperature/H2O_pressure)

I think there are a lot of places where this will particularly benefit ImSim.

@cwwalter
Copy link
Member

This is great Josh.

FYI, here is an old imSim issue about this:

LSSTDESC/imSim#48

@cwwalter cwwalter mentioned this pull request Sep 17, 2024
Open
rmjarvis
rmjarvis approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@rmjarvis rmjarvis left a comment

Choose a reason for hiding this comment

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

Looks great Josh! Just a couple minor doc errors.

docs/config_values.rst Outdated
* A string interpretable by `astropy.units.Quantity` (e.g. '8.7 m')
* A dict with:

* ``type`` = *str* (required) There is only one valid option:
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be two valid options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I mimicked the sky_value section, which also technically has two valid options. I'll fix that one too.

docs/config_values.rst Outdated
* A string interpretable by `astropy.units.Unit` (e.g. 'm')
* A dict with:

* ``type`` = *str* (required) There is only one valid option:
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

@jmeyers314 jmeyers314 merged commit 5ee3c0d into main Sep 18, 2024
10 checks passed
@jmeyers314 jmeyers314 deleted the quantity branch September 18, 2024 16:18
@rmjarvis rmjarvis added notation/conventions Related to choices of units, names of things, other semi-arbitrary conventions feature request Request for a new feature in GalSim config Related to the config-processing functionality. labels Sep 18, 2024
@rmjarvis rmjarvis added the desc Of possible interest to LSST DESC members looking for a project label Sep 18, 2024
@rmjarvis rmjarvis added this to the v2.6 milestone Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmjarvis rmjarvis rmjarvis approved these changes

Assignees
No one assigned
Labels
config Related to the config-processing functionality. desc Of possible interest to LSST DESC members looking for a project feature request Request for a new feature in GalSim notation/conventions Related to choices of units, names of things, other semi-arbitrary conventions
Projects
None yet
Milestone
v2.6
Development

Successfully merging this pull request may close these issues.
3 participants
@jmeyers314 @cwwalter @rmjarvis