Mapcat integration for make_atomic_filterbin_map and make_coadd_atomic_map by davidvng · Pull Request #1534 · simonsobs/sotodlib

davidvng · 2026-01-22T21:32:57Z

Modifies existing database querying and writing from original sqlite structure to using mapcat for both make_atomic_filterbin_map and make_coadd_atomic_map

Related changes will be needed in the site-pipeline-configs, especially to set the following environment variables used by mapcat:

MAPCAT_DATABASE_NAME
MAPCAT_DATABASE_TYPE
MAPCAT_ATOMIC_PARENT
MAPCAT_ATOMIC_COADD_PARENT

I've tested the coadd script and confirmed everything works as intended.

However, for the atomic mapmaking, I have not tested due to permissions issues. I'd appreciate @chervias or @JBorrow to double check the changes in make_atomic_filterbin_map.py make sense.

JBorrow · 2026-01-22T22:07:48Z

Not sure why the tests are failing due to the missing dependency.

iparask · 2026-01-23T14:09:23Z

Hello @davidvng, take a look here to see how mapcat (#1485) is integrated with the depth1 mapmaker. There we pass the config as an input. In addition you do not really need select as the session provides a query method which is as easy to invoke as select.

JBorrow · 2026-01-23T14:12:34Z

@iparask using session.query() is deprecated. Forming a select statement is preferred behaviour and is the standard in SQLAlchemy 2.0+. See: https://docs.sqlalchemy.org/en/21/changelog/migration_20.html#migration-orm-usage

iparask · 2026-01-23T14:18:34Z

+        with settings.session() as session:
+            data = AtomicMapCoaddTable(
+                coadd_name=map_name,
+                prefix_path=prefix_path,
+                platform=platform,
+                interval=interval,
+                start_time=start_time,
+                stop_time=stop_time,
+                freq_channel=band,
+                geom_file_path=self.geom_file_path,
+                split_label=split_label,
+                atomic_maps=self.maps if self.coadd_atomic else [],
+                parent_coadds=self.maps if not self.coadd_atomic else []
+            )
+
+            session.add(data)
+            session.commit()


I would place this in a function similar to the depth 1 map in the PR I referenced

iparask · 2026-01-23T14:19:19Z

+        with settings.session() as session:
+            if '+' in band:
+                statement = select(AtomicMapTable).where((AtomicMapTable.telescope == platform) & (AtomicMapTable.split_label == split_label) & (AtomicMapTable.ctime >= start_time) & (AtomicMapTable.ctime <= stop_time))
+            else:
+                statement = select(AtomicMapTable).where((AtomicMapTable.telescope == platform) & (AtomicMapTable.freq_channel == band) & (AtomicMapTable.split_label == split_label) & (AtomicMapTable.ctime >= start_time) & (AtomicMapTable.ctime <= stop_time))
+            maps = session.execute(statement).scalars().all()
+            map_ids = [m.atomic_map_id for m in maps]


The same as above

iparask · 2026-01-23T14:20:47Z

+        with settings.session() as session:
+            statement = select(AtomicMapCoaddTable).where((AtomicMapCoaddTable.interval == interval) & (AtomicMapCoaddTable.platform == platform) & (AtomicMapCoaddTable.freq_channel == band) & (AtomicMapCoaddTable.split_label == split_label) & (AtomicMapCoaddTable.start_time == start_time) & (AtomicMapCoaddTable.stop_time == stop_time)).order_by(AtomicMapCoaddTable.coadd_id.desc())
+            coadd = session.execute(statement).scalars().first()
+            if coadd is not None:
+                atomic_parent_ids = [a.atomic_map_id for a in coadd.atomic_maps]
+                coadd_parent_ids = [c.coadd_id for c in coadd.parent_coadds]
+
+                if (map_ids == atomic_parent_ids) or (map_ids == coadd_parent_ids):
+                    return False
+


This can also be a function under mapcat.py

davidvng · 2026-02-02T18:34:27Z

Not sure why the tests are failing due to the missing dependency.

@JBorrow The tests are still failing and I'm assuming it has to do with mapcat being in the optional dependencies? Thoughts on moving it to the core list of dependencies?

iparask · 2026-02-02T18:58:53Z

Not sure why the tests are failing due to the missing dependency.

@JBorrow The tests are still failing and I'm assuming it has to do with mapcat being in the optional dependencies? Thoughts on moving it to the core list of dependencies?

I am not sure that is the solution. #759 passes with mapcat as an optional dependency. It might be some other reason. There is similar discussion on slack.

davidvng · 2026-02-04T19:26:25Z

@JBorrow @iparask Looks like the python-3.13 tests now pass, but the python-3.10 tests will always fail since mapcat doesn't support 3.10.

JBorrow · 2026-02-04T21:35:39Z

0.1.1 includes 'support' for 3.10.

iparask · 2026-02-13T19:41:51Z

+        with Settings(**mapcat_settings).session() as session:
+            data = AtomicMapCoaddTable(
+                coadd_name=map_name,
+                prefix_path=prefix_path,
+                platform=platform,
+                interval=interval,
+                start_time=start_time,
+                stop_time=stop_time,
+                freq_channel=band,
+                geom_file_path=self.geom_file_path,
+                split_label=split_label,
+                atomic_maps=self.maps if self.coadd_atomic else [],
+                parent_coadds=self.maps if not self.coadd_atomic else []
+            )
+
+            session.add(data)
+            session.commit()


This should be outside of the mapmaking module. Think what will happen if we decide to change mapcat with something else. We will have to change the code here as well.

Please move any database interactions in the site_pipeline scripts, and create a file for these interactions under the utils of site_pipeline to keep them.

iparask · 2026-02-13T19:42:50Z

+        with Settings(**mapcat_settings).session() as session:
+            statement = select(AtomicMapCoaddTable)
+            statement = statement.where(AtomicMapCoaddTable.interval == lower_interval)
+            statement = statement.where(AtomicMapCoaddTable.platform == platform)
+            statement = statement.where(AtomicMapCoaddTable.freq_channel == band)
+            statement = statement.where(AtomicMapCoaddTable.split_label == split_label)
+            statement = statement.where(AtomicMapCoaddTable.start_time >= start_time)
+            statement = statement.where(AtomicMapCoaddTable.stop_time <= stop_time)
+            maps = session.execute(statement).scalars().all()
+            map_ids = [m.coadd_id for m in maps]



As I said before this script needs to be database agnostic. You can get the map_ids before and pass them as an argument.

Apologies, could you clarify what you mean by this? Do you mean to create map names based on the provided info to query and get the map_ids? I don't understand why this is needed when this has been tested and works.

As for the atomic maps, I understand you can use mapmaking.build_obslists to create the map names as you do in your depth1 script. However, in this case, the make_atomic_filterbin_map.py script, which runs separately, already does this and is the script that commits to the database. I don't understand the need to repeat that in this coadd script when the entries for atomic maps already exist.

iparask · 2026-02-13T19:43:04Z

+        with Settings(**mapcat_settings).session() as session:
+            statement = select(AtomicMapTable)
+            statement = statement.where(AtomicMapTable.telescope == platform)
+            statement = statement.where(AtomicMapTable.split_label == split_label)
+            statement = statement.where(AtomicMapTable.ctime >= start_time)
+            statement = statement.where(AtomicMapTable.ctime <= stop_time)
+            if '+' not in band:
+                statement = statement.where(AtomicMapTable.freq_channel == band)
+            maps = session.execute(statement).scalars().all()
+            map_ids = [m.atomic_map_id for m in maps]


Yeah I'm not sure what this means either, can you clarify how this is not datbaase agnostic?

The mapmaker itself knows about the database, right? It is not so much the type of database as it is the fact that it needs access to one. I do understand with the site_pipeline script needs to have access to the database, but not the mapmaker itself.

I would expect the mapmaker to take the information it needs to create the coadd, how they arrived or selected is the responsibility of the script that calls the mapmaker.

iparask · 2026-02-13T19:43:17Z

+        with Settings(**mapcat_settings).session() as session:
+            statement = select(AtomicMapCoaddTable)
+            statement = statement.where(AtomicMapCoaddTable.interval == interval)
+            statement = statement.where(AtomicMapCoaddTable.platform == platform)
+            statement = statement.where(AtomicMapCoaddTable.freq_channel == band)
+            statement = statement.where(AtomicMapCoaddTable.split_label == split_label)
+            statement = statement.where(AtomicMapCoaddTable.start_time == start_time)
+            statement = statement.where(AtomicMapCoaddTable.stop_time == stop_time)
+            statement = statement.order_by(AtomicMapCoaddTable.coadd_id.desc())
+            coadd = session.execute(statement).scalars().first()
+            if coadd is not None:
+                atomic_parent_ids = [a.atomic_map_id for a in coadd.atomic_maps]
+                coadd_parent_ids = [c.coadd_id for c in coadd.parent_coadds]
+


…omic-mapcat

iparask · 2026-03-04T19:24:36Z

This line is left there from the direct integration with sqlite.

iparask · 2026-03-04T23:50:55Z

It seems that this is not used anymore. I think you can remove it.

* fix: mapcat integration with atomic maps * fix: no need to return everything

iparask · 2026-04-22T15:45:15Z

@JBorrow, @chervias can you also take a look here?

JBorrow · 2026-04-23T19:50:34Z

+        mapcat_database_name: str = os.environ.get('MAPCAT_DATABASE_NAME', ''),
+        mapcat_database_type: str = os.environ.get('MAPCAT_DATABASE_TYPE', ''),
+        mapcat_atomic_parent: str = os.environ.get('MAPCAT_ATOMIC_PARENT', ''),


You already have mapcat settings here so just use that, it reads the environment for you.

JBorrow · 2026-04-23T19:51:20Z

Other than the duplicated reading of environment variables (you should use Settings from mapcat as the variables may be specified in another manner) this looks good.

davidvng added 4 commits January 22, 2026 20:02

atomic coadd mapcat integration

ca4eebc

atomic filterbin mapcat integration

7afe8f1

import session cleanup

8d1de9f

fix query typo

f447c02

davidvng requested review from JBorrow and chervias January 22, 2026 21:32

add mapcat module

178f7d5

JBorrow reviewed Jan 22, 2026

View reviewed changes

Comment thread sotodlib/mapmaking/coadd_mapmaker.py Outdated

iparask reviewed Jan 23, 2026

View reviewed changes

cleanup statements

a69c373

davidvng added 2 commits February 2, 2026 19:09

Merge branch 'master' into atomic-mapcat

6b4b56b

Merge branch 'master' into atomic-mapcat

7a9f308

iparask mentioned this pull request Feb 4, 2026

Include python3.10 simonsobs/mapcat#17

Merged

davidvng added 5 commits February 9, 2026 20:09

Merge branch 'master' into atomic-mapcat

694d99b

using mapcat settings from config file; various cleanup

35d05c8

remove print statements

9023240

use mapcat settings from configs for atomic mapmaking

dbe1a8b

Merge branch 'master' into atomic-mapcat

96c252c

davidvng requested review from JBorrow and iparask February 9, 2026 22:49

iparask requested changes Feb 13, 2026

View reviewed changes

davidvng added 2 commits March 3, 2026 20:57

Merge branch 'master' into atomic-mapcat

bbaf515

Merge branch 'atomic-mapcat' of github.com:simonsobs/sotodlib into at…

dc0d756

…omic-mapcat

iparask reviewed Mar 4, 2026

View reviewed changes

iparask and others added 3 commits March 9, 2026 14:50

Integrating mapcat with atomic maps (#1569)

5576c3e

* fix: mapcat integration with atomic maps * fix: no need to return everything

fix params

2a8edc1

remove unused sqlite3

dab1b84

iparask self-requested a review April 22, 2026 15:44

JBorrow reviewed Apr 23, 2026

View reviewed changes

Conversation

davidvng commented Jan 22, 2026

Uh oh!

JBorrow commented Jan 22, 2026

Uh oh!

Uh oh!

iparask commented Jan 23, 2026

Uh oh!

JBorrow commented Jan 23, 2026

Uh oh!

iparask Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidvng commented Feb 2, 2026

Uh oh!

iparask commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidvng commented Feb 4, 2026

Uh oh!

JBorrow commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iparask Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iparask commented Apr 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JBorrow commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iparask Jan 23, 2026 •

edited

Loading

iparask commented Feb 2, 2026 •

edited

Loading

iparask Mar 20, 2026 •

edited

Loading