DM-45860: Expand support for SIAv2 and VOTable #18

timj · 2024-08-27T18:52:32Z

Requires lsst/daf_butler#1066 and lsst/daf_butler#1087.

codecov · 2024-08-27T19:02:56Z

Codecov Report

Attention: Patch coverage is 92.43570% with 50 lines in your changes missing coverage. Please review.

Project coverage is 82.30%. Comparing base (6892e05) to head (06a2bc1).
Report is 72 commits behind head on main.

Files with missing lines	Patch %	Lines
python/lsst/dax/obscore/siav2.py	93.09%	9 Missing and 10 partials ⚠️
python/lsst/dax/obscore/obscore_exporter.py	88.42%	10 Missing and 4 partials ⚠️
python/lsst/dax/obscore/script/obscore_siav2.py	64.28%	5 Missing ⚠️
python/lsst/dax/obscore/script/obscore_export.py	33.33%	4 Missing ⚠️
python/lsst/dax/obscore/config.py	93.33%	2 Missing and 1 partial ⚠️
tests/test_siav2.py	98.07%	1 Missing and 1 partial ⚠️
tests/test_where.py	93.33%	1 Missing and 1 partial ⚠️
python/lsst/dax/obscore/cli/cmd/commands.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #18       +/-   ##
===========================================
+ Coverage   62.03%   82.30%   +20.27%     
===========================================
  Files          11       16        +5     
  Lines         461     1074      +613     
  Branches      107      276      +169     
===========================================
+ Hits          286      884      +598     
+ Misses        166      165        -1     
- Partials        9       25       +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

python/lsst/dax/obscore/siav2.py

andy-slac

Jsu commenting, as this is still a draft PR. Looks good, one concern is handling of regexes for collection names in obscore config, this is not going to work with the new query system. And there are assumptions about dimension names, but you know this already.

configs/ci_hsc_gen3.yaml

python/lsst/dax/obscore/cli/cmd/commands.py

python/lsst/dax/obscore/config.py

python/lsst/dax/obscore/siav2.py

python/lsst/dax/obscore/obscore_exporter.py

timj · 2024-09-03T20:15:50Z

And there are assumptions about dimension names,

I designed it so that the assumptions about dimension names are isolated in SIAv2Handler. In the future the handler class will be a parameter of the SIAv2 obscore configuration.

Butler ObsCore itself is a much bigger issue though because I'm sure "visit" is assume all over the place.

Comes from sdm_schemas

Uses felis to describe the table metadata.

Supports POS, BAND, TIME, INSTRUMENT

Now track the WHERE and bind information discretely and combine as needed.

Use WhereBind class to specify the specific query for export.

Otherwise every query will reload the YAML file.

python/lsst/dax/obscore/obscore_exporter.py

Rather than diverting through astropy Table to get the masked array, convert to Arrow Table and use Eli Rykoff's conversion code in daf_butler to generate the masked array directly.

andy-slac

Looks great, few minor comments.

python/lsst/dax/obscore/cli/cmd/commands.py

python/lsst/dax/obscore/config.py

python/lsst/dax/obscore/obscore_exporter.py

python/lsst/dax/obscore/siav2.py

andy-slac · 2024-10-01T20:59:10Z

python/lsst/dax/obscore/siav2.py

+
+        Returns
+        -------
+        info : `dict` [ `str`, `typing.Any` ]


Maybe make a simple dataclass or NamedTuple for this return type to make it type-safer?

The contents are universe-dependent. In our case we are returning a dict of filters and bands that match the query parameters. SphereX won't be returning that though since they don't have physical filters in their data model. I suppose a could define an empty BandInfo model and then return a subclass in this method but that wouldn't really help with the type safety problem since "filters" and "bands" would be properties of the subclass.

andy-slac · 2024-10-01T21:06:33Z

python/lsst/dax/obscore/siav2.py

+        region = regions[0]
+        if len(regions) > 1:
+            for r in regions[1:]:
+                region = UnionRegion(region, r)


Would be nice if UnionRegion could take any number of regions in its constructor.

Indeed it would.

python/lsst/dax/obscore/siav2.py

tests/test_siav2.py

python/lsst/dax/obscore/obscore_exporter.py

Co-authored-by: stvoutsin <steliosvoutsinas@yahoo.com>

Co-authored-by: Andy Salnikov <salnikov@slac.stanford.edu>

This is sometimes fine but when combining a where="" with another where we were getting "x = y AND ()" which is invalid. In this commit we trap and raise. Another option is to ignore it when the combining happens.

python/lsst/dax/obscore/siav2.py

Co-authored-by: stvoutsin <steliosvoutsinas@yahoo.com>

timj force-pushed the tickets/DM-45860 branch 2 times, most recently from 5f3c642 to 44d39ea Compare August 27, 2024 18:58

timj force-pushed the tickets/DM-45860 branch 5 times, most recently from 75b80cc to 1e534cc Compare August 29, 2024 19:52

timj commented Aug 29, 2024

View reviewed changes

python/lsst/dax/obscore/siav2.py Show resolved Hide resolved

timj force-pushed the tickets/DM-45860 branch 4 times, most recently from 44661ba to 9b66b4f Compare August 31, 2024 00:21

andy-slac reviewed Sep 2, 2024

View reviewed changes

timj force-pushed the tickets/DM-45860 branch 2 times, most recently from 69adc61 to 73c37d3 Compare September 10, 2024 15:56

timj mentioned this pull request Sep 16, 2024

DM-46363: Allow ObscoreExporter to work without SqlRegistry #19

Merged

timj added 12 commits September 18, 2024 09:46

Ignore _build files

802c1db

Simple config suitable for ci_hsc_gen3 repo

04910d3

Add ObsCore felis definition

4a31061

Comes from sdm_schemas

Add VOTable exporter option

4bdef29

Uses felis to describe the table metadata.

First SIAv2 prototype

d8dc4be

Supports POS, BAND, TIME, INSTRUMENT

Reorganize SIA query to support multiple instruments

dc9e786

Now track the WHERE and bind information discretely and combine as needed.

Allow for verbose log level

2b7afc6

Add EXPTIME support

88ea57b

Move SIAv2 query handling into single method

295a8a7

Use WhereBind class to specify the specific query for export.

Use new DimensionGroup API to get relevant time/region dimension

fab2dc1

Add do not merge check workflow

0f90581

Rearrange logic to allow "exposure" queries to work with regions

61d1660

timj added 2 commits September 23, 2024 21:52

Cache the Felis schema

98f463d

Otherwise every query will reload the YAML file.

Add explicit SIAv2 test with known and unknown instrument

f625c28

timj commented Sep 25, 2024

View reviewed changes

python/lsst/dax/obscore/obscore_exporter.py Outdated Show resolved Hide resolved

timj added 3 commits September 30, 2024 07:08

Simplify the is None mask check

b634e8e

Rewrite the VOTable construction code to use arrow_to_numpy

e253bc1

Rather than diverting through astropy Table to get the masked array, convert to Arrow Table and use Eli Rykoff's conversion code in daf_butler to generate the masked array directly.

Stop using deprecated mambaforge in action

007f438

timj marked this pull request as ready for review September 30, 2024 23:34

andy-slac approved these changes Oct 1, 2024

View reviewed changes

stvoutsin reviewed Oct 2, 2024

View reviewed changes

python/lsst/dax/obscore/obscore_exporter.py Show resolved Hide resolved

timj and others added 2 commits October 3, 2024 13:58

Explicitly copy the config for testing before setting batch size

dfbeb70

Add utype to output

20ec130

Co-authored-by: stvoutsin <steliosvoutsinas@yahoo.com>

timj force-pushed the tickets/DM-45860 branch from 0185d4f to 20ec130 Compare October 3, 2024 20:58

timj and others added 4 commits October 3, 2024 14:27

Fix some typos

3056da8

Co-authored-by: Andy Salnikov <salnikov@slac.stanford.edu>

fixup! Cache the Felis schema

6c2a4f7

fixup! Modify the SIAv2Parameters handling to allow multiples

2c0013f

Add some tests for WhereBind and allow identical duplicate binds

81ea21c

timj force-pushed the tickets/DM-45860 branch from 4963604 to 81ea21c Compare October 3, 2024 23:21

timj added 4 commits October 3, 2024 16:32

Remove unnecessary results variable

dfb2e2c

Fix parameter types in siav2 script API docstrings

443c1cf

Use model_copy to copy config

0cb2a19

Trap when WhereBind.combine is called with nothing

ded600c

This is sometimes fine but when combining a where="" with another where we were getting "x = y AND ()" which is invalid. In this commit we trap and raise. Another option is to ignore it when the combining happens.

stvoutsin reviewed Oct 4, 2024

View reviewed changes

python/lsst/dax/obscore/siav2.py Outdated Show resolved Hide resolved

timj added 2 commits October 3, 2024 22:19

Handle empty wheres that can now come from exptime calculation

f3d99e5

Return overflow status rather than use object property

5f7f6e1

timj force-pushed the tickets/DM-45860 branch from 3cef369 to 5f7f6e1 Compare October 4, 2024 15:08

timj and others added 3 commits October 4, 2024 08:22

Use single limit variable in loop

b0ed10d

Make SIAv2Parameters public

6e5a58f

Co-authored-by: stvoutsin <steliosvoutsinas@yahoo.com>

Add a comment to the ci_hsc_gen3 config

06a2bc1

timj merged commit 5919c78 into main Oct 4, 2024
14 checks passed

timj deleted the tickets/DM-45860 branch October 4, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-45860: Expand support for SIAv2 and VOTable #18

DM-45860: Expand support for SIAv2 and VOTable #18

timj commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

andy-slac left a comment

timj commented Sep 3, 2024

andy-slac left a comment

andy-slac Oct 1, 2024

timj Oct 3, 2024

andy-slac Oct 1, 2024

timj Oct 3, 2024

DM-45860: Expand support for SIAv2 and VOTable #18

DM-45860: Expand support for SIAv2 and VOTable #18

Conversation

timj commented Aug 27, 2024 • edited Loading

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

andy-slac left a comment

Choose a reason for hiding this comment

timj commented Sep 3, 2024

andy-slac left a comment

Choose a reason for hiding this comment

andy-slac Oct 1, 2024

Choose a reason for hiding this comment

timj Oct 3, 2024

Choose a reason for hiding this comment

andy-slac Oct 1, 2024

Choose a reason for hiding this comment

timj Oct 3, 2024

Choose a reason for hiding this comment

timj commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading