Add TableRow #472
Add TableRow #472
Conversation
imsim/exposure_table.py
Outdated
| type_name="ExposureData", | ||
| gen_func=ExposureData, | ||
| valid_types=[float, int, bool, str, galsim.Angle, list], | ||
| input_type="exposure_table", |
There was a problem hiding this comment.
The "Exposure" part of the name seems too specific to your use case. This feels like it should be more generic, like AstropyTableRow or QTableRow or even just TableRow. Something like that. The implementation doesn't have anything to do with exposures.
It also took me a couple reads to understand that you're just pulling out a single row from the full table. So it feels like that should be part of the name.
There was a problem hiding this comment.
Sounds good. I think I actually had AstropyTableRow at one point during development and changed it (can't remember why now). I landed on TableRow and RowData now.
tests/test_exposure_table.py
Outdated
| galsim.config.RemoveCurrent(config["input"]["exposure_table"]) | ||
| galsim.config.ProcessInput(config) | ||
| idx = idx0 * 2 + idx1 | ||
| check_exp_data(config, idx) |
There was a problem hiding this comment.
Rather than just checking explicit calls to ExposureData, it's probably also worth adding some values in the config itself that use the input type. Things that use { 'type': 'ExposureData', 'field': ... }, since that's the syntax that users will actually use.
There was a problem hiding this comment.
Sure. I added a test that actually creates a batoid telescope, similar to what I'm doing for OR4.
jmeyers314
force-pushed
the
exposure_table
branch
from
4e41779
to
92aea32
Compare
Sure, ImSim is fine for now. Can always migrate later if that's useful. |
|
Assuming this passes CI, I think it's ready for another look @rmjarvis . |
jmeyers314
force-pushed
the
exposure_table
branch
from
20526b4
to
31fef80
Compare
Adds a custom InputType for the galsim config machinery to parse an astropy QTable. My current use case is to grab one row of the table for each AOS operations rehearsal 4 (OR4) exposure (example here), so this is similar to the OpsimData class. I wanted to be able to leverage units and structured array columns though, and lose some of the other peculiarities of OpSimData.
It's possible this would fit in better in GalSim than ImSim. Wherever it ends up, it should be part of a tag/release soonish I think for OR4 reproducibility.