Provenances #195

ColeFMeldorf · 2025-10-27T19:35:41Z

In this PR we implement the specific provenance and database updates that have been rolled out in snappl over the past few weeks. To do so we:

NOTE: Some tests still use ou2024 as their image source, which I know can't stay. This is due to some oversight on my end where I accidentally made these tests source reliant. They will be fixed but this PR is already huge. Issue is already raised on this matter.

rknop

There are still some fundamental issues that need to be addressed.

rknop · 2025-10-28T17:44:11Z

campari/campari_runner.py

+                                                 ra=self.ra, dec=self.dec, mjd_discovery_min=self.transient_start,
+                                                 mjd_discovery_max=self.transient_end)
+            else:
+                provenance_tag = "ou2024"


provenance_tag and process need to not be hardcoded, but need to be something that you pass on the command line as arguments.

This should be done right away, because these hardcoded values, I believe, are not what we're going to use for the Nov test.

rknop · 2025-10-28T17:50:42Z

campari/campari_runner.py

                           f" dec={diaobj.dec}, transient_start={diaobj.mjd_start}, transient_end={diaobj.mjd_end}")
            image_list = self.get_exposures(diaobj)
-            sedlist = self.get_sedlist(diaobj.id, image_list)
+            sedlist = self.get_sedlist(diaobj.name, image_list)


diaobj.name is not a reliable attribute to use.

This will work for the current database, but may well not work in the Nov test, because I don't know what names will be saved from sidecar, or if any will be saved.

get_sedlist itself is a problem, and will need to be fixed ASAP (though not in this PR). It will not work for anything other than ou2024 truth objects -- including objects found from sidecar. So, it's not nov2025 ready.

We have a lot of sed development we need to do. But, in the short term, you need to have it have a fallback (flat SED?) when we're not doing ou2024 truth objects.

Ok sounds good, in the meantime I wrapped get_sedlist in a try except block so that if it gets something bizarre for an ID it will return a flatSED instead.

Ultimately we're going to have to think about getting object SEDs from snappl, I think. We need to sit down as photometry in general and talk about how we're going to handle SEDs, because it's nontrivial when you don't have truth files.

I raised an issue in snappl!

rknop · 2025-10-28T17:53:00Z

campari/data_construction.py

+    if image_source != "ou2024":
+        provenance_tag = "ou2024"
+        process = "load_ou2024_image"
+    else:
+        provenance_tag = None
+        process = None


This cannot be hardcoded.

You need to pass provenance_tag and process in as arguments.

You don't need the if/else ; just make the default values of the arguments None.

But, ultimately, you need to pass on the command line:

diaobject provenance_tag and process

image collection provenance_tag and process

You then need to feed those values around to where they're needed.

Never hardcode anything like this, even if it's just to get the first thing going. It will just make life harder in the future... and that future may be only two weeks off.

rknop · 2025-10-28T17:54:43Z

campari/data_construction.py

+    if image_source == "ou2024":
+        img_collection_prov = None
+    else:
+        img_collection_prov = img_collection.provenance


Also, as a general rule -- if you find yourself interpreting the value of image_source (by which I assume you mean the collection?), you're doing it wrong.

Things like this need to be opaque to campari, and just passed on to snappl. If snappl doesn't fully support what you need, we need to fix it there. But, the more you code in things in campari that look at these values, the more trouble we're going to have in the future.

If you MUST, just to get things done, you need to put in commens that this is a bad and needs to be fixed, and it needs to be referenced in an issue (making a new issue if there isn't one already).

rknop · 2025-10-28T17:57:11Z

campari/io.py

+        params=cfg,  # keepkeys=["photometry.campari"],
+        omitkeys=["photometry.campari.galsim", "photometry.campari.simulations"],


This isn't what you want, because as is right now, you will include all of the system tree, which is explicitly what you don't want

What you want is keepkeys=['photometry.campari']. If there are then things unerneath photometry.campari that aren't supposed to be part of the config, they should be moved to somewhere else. (What's in phtoometry.campari.galsim and photometry.camapri.simulations? If you are building lightcurves from simulations and you are saving them, you absolutely want those as part of the params.)

Those simulations are only used for making my own images so that campari can then be run on them, but they wouldn't be used at all in a production run, so I am decently certain I don't want to include them, is that correct?

I moved the simulation section so that it is not included.

rknop · 2025-10-28T17:58:48Z

campari/io.py

-    return QTable(data=data_dict, meta=meta_dict, units=units)
+    SNLogger.debug("trying to build a lightcurve object")
+    meta_dict["band"] = band  # I don't ever expect campari to do multi-band fitting so just store the one band.
+    meta_dict["diaobject_position_id"] = "e98e579f-0ab3-4ad6-8042-2606d7d53014"  # placeholder for now XXX TODO


Make an issue for this, but also, as is this will be a problem. Putting in a made up diaobject position id will prevent you from being able to save lightcurves to the database. The database will try to look up the diaobject position based on the id you give it, and will reject the lightcurve record of that diaboject position id doesn't exist.

If you're just using the position from DiaObject right now, this should be None.

If you're pulling an improved diaobject position from the database (which you need to do -- but if you don't do it yet, make an issue and save it for a future PR), then just use that position's ID.

Issue made!

Issue made.

rknop · 2025-10-28T18:00:20Z

campari/io.py

+    band_map = {
+            "r": "R062",
+            "z": "Z087",
+            "y": "Y106",
+            "j": "J129",
+            "h": "H158",
+            "f": "F184",
+            "w": "W146",


This scares me a little bit. Where are "r", "z", et al. coming in to it?

I realized I made a mistake and had a [0] after band, just giving a single letter. I mistakenly thought that meant Lightcurve was abbreviating bands. This has been removed and the dictionary removed as well.

rknop · 2025-10-28T18:02:26Z

campari/RomanASP.py

                             "Default is 'ou24', the Open Universe 2024 catalog. 'manual'"
                             "will use the input ra and dec given by the user, and not perform any lookup.")
-    parser.add_argument("--image_source", type=str, default="ou2024", required=False,
+    parser.add_argument("--image_source", type=str, default="snpitdb", required=False,


As I noted above, you will need to add six more arguments here:

Image collection provenance tag and process

Object provenance tag and process

Object position provenance tag and process

All of these can default to None. (You will get errors from snappl if you try to get image collections or objects without a provenance when using a database image collection; that's fine, let snappl throw the error, you shouldn't verify this yourself.)

You will want to look inside the object position provenance tag; if that is none, use the RA and Dec from the object. If not, then get the object position using the method in DiaObject that does that.

Sorry when you say "get the RA/dec from the object" do you mean that I should do a manual lookup?

No; if dobj is a DiaObject, then I mean dobj.ra and dobj.dec.

Ideally, though, you will be calling the get_position method of a DiaObject to get an improved position; see the docstring for that.

examples/perlmutter/campari_config_test.yaml

rknop · 2025-10-28T18:03:58Z

examples/perlmutter/interactive_podman.sh

    --mount type=bind,source=$PWD,target=/home \
    --mount type=bind,source=$PWD/campari,target=/campari \
    --mount type=bind,source=$PWD/campari_out_dir,target=/campari_out_dir \
+    --mount type=bind,source=$PWD/campari_out_dir,target=/lightcurves \


... did this actually work? I don't see a bind-mount for /secrets . (I may be blind.)

Oops I accidentally had two copies of interactive_podman.sh for some reason. Secrets was present in the other one but I have now updated this one too.

ColeFMeldorf · 2025-10-28T21:25:07Z

Note to self: object position prov still needs to be added to upstreams

ColeFMeldorf · 2025-10-28T21:29:11Z

Note to self: make sure I can write to parquet files

rknop

Lots more comments

rknop · 2025-10-29T16:06:16Z

campari/campari_runner.py

        self.prebuilt_static_model = kwargs["prebuilt_static_model"]
        self.prebuilt_transient_model = kwargs["prebuilt_transient_model"]

+        self.find_object_provenance_tag = kwargs["find_obj_prov_tag"]


I would recommend you and Lauren agree on this; my preference would be the command-line arguments I gave Lauren, that I think you've seen.

rknop · 2025-10-29T16:07:55Z

campari/campari_runner.py

+            if self.object_collection == "manual":
+                diaobjs = DiaObject.find_objects(collection=self.object_collection, dbclient=self.dbclient,
+                                                 provenance_tag=self.find_object_provenance_tag,
+                                                 process=self.find_object_process, name=ID,


name=ID is a problem here... as I noted, name is not a reliable property, and there is also an id property.

I would clean this up so that you have both name and id (or object_id) as something that can be passed. In practice, we're probably going to be passing around ids more often than names. But definitely do not conflate the two.

rknop · 2025-10-29T16:09:28Z

campari/campari_runner.py

                           f" dec={diaobj.dec}, transient_start={diaobj.mjd_start}, transient_end={diaobj.mjd_end}")
            image_list = self.get_exposures(diaobj)
-            sedlist = self.get_sedlist(diaobj.id, image_list)
+            sedlist = self.get_sedlist(diaobj.name, image_list)


Ultimately we're going to have to think about getting object SEDs from snappl, I think. We need to sit down as photometry in general and talk about how we're going to handle SEDs, because it's nontrivial when you don't have truth files.

rknop · 2025-10-29T16:10:19Z

campari/campari_runner.py

+                                                                pointing_list=self.pointing_list,
+                                                                dbclient=self.dbclient,
+                                                                collection_provenance_tag=self.
+                                                                get_collection_provenance_tag,


Although you use it with ImageCollection, these are really image provenance tags and image processes.

This ties in with the renaming of the command-line arguments that I think you should do.

rknop · 2025-10-29T16:12:23Z

campari/campari_runner.py

        """Create a list of SEDs for the given SNID and images."""
-        sed_obj = OU2024_Truth_SED(ID, isstar=(self.object_type == "star")) if self.fetch_SED else Flat_SED()
+        try:
+            sed_obj = OU2024_Truth_SED(ID, isstar=(self.object_type == "star")) if self.fetch_SED else Flat_SED()


Again, be careful with ID here. I would rename this parameters.

In the database, id is something like bcb579f5-c3b6-4c70-a128-bb38bc88899c. The thing that is like 20172782 is the name of the object... and as I've said, not all objects are even going to have names.

For now, I'd just make this argument name. This whole method needs to be fixed once photometry decides how we're going to handle SEDs.

Done, and I've raised that issue to talk about SEDs.

rknop · 2025-10-29T16:22:13Z

campari/io.py

-    SNLogger.info(f"Saving lightcurve to {lc_file}")
-    lc.write(lc_file, format="ascii.ecsv", overwrite=overwrite)
+    lc.write(
+        base_dir=output_path, filepath=f"{identifier}_{band}_{psftype}_lc.ecsv", filetype="ecsv", overwrite=overwrite


Don't specify a filepath here. Let snappl give you the filepath.

After you call .write, the filepath attribute of lc will have the relative filepath that snappl chose.

Also edit the docstring to remove the specification of what the output path is, since campari won't control that.

As per our slack conversation, this now only has a filepath if not saving to the database!

rknop · 2025-10-29T16:23:20Z

campari/io.py

    """This function parses settings in the SMP algorithm and saves the
    lightcurve to an ecsv file with an appropriate name.
    Input:
    lc: the lightcurve data


What is the expected type of lc ? Is it a Lightcurve object? If so, specify that here.

rknop · 2025-10-29T16:24:31Z

campari/io.py

    band (str): the bandpass of the images used
    psftype (str): "romanpsf" or "analyticpsf"
    output_path (str): the path to save the lightcurve to.  Defaults to
      config value phtometry.campari.paths.output_dir


This is not the right config value to use. You don't want it under photometry.campari, because now you've got something system specific, and nothing system specific is supposed to be anywhere other than system.

The right thing to use is system.paths.lightcurves.

Got it. I've made system.paths.lightcurves point to /campari_out_dir for being able to reproduce tests, is that ok?

For tests, that's fine, though longer term we're going to want to make it somewhere else. We'll do that when we set up your tests to have a self-contained environment. (Did you make an issue for that?) This is post-November stuff.

I have now!

rknop · 2025-10-29T16:25:09Z

campari/io.py

-    band = lc["filter"][0]
+    band = lc.meta["band"]
+    SNLogger.debug(f"saving lightcurve for id={identifier}, band={band}, psftype={psftype}")
    output_path = Config.get().value("photometry.campari.paths.output_dir") if output_path is None else output_path


Note above : make this system.paths.lightcurves

I might also name the variable something like base_output_path, to make it clear that this is the base directory, not the full output path.

Done and done!

campari/RomanASP.py

ColeFMeldorf · 2025-10-29T20:20:29Z

Note to self: make sure I am using diaobj to lookup object positions before this is merged

rknop

Getting closer. Still a few small things to resolve.

I should note that in triaging my time, I haven't really looked at the tests. I reserve the right in future PRs to say we have to redo all of those.... (In fact, you already know we need to redo some, because eventually you're going to need to work with a self-contained test environment.)

rknop · 2025-10-30T13:30:05Z

campari/campari_runner.py

                             "the galaxy be a delta function.")

+        # Lightcurve provenance argument parsing logic:
+        SNLogger.debug("no save to db is set to " + str(kwargs["save_to_db"]))


Do you mean SNLogger.debug("save to db...? (I don't get the 'no')

rknop · 2025-10-30T13:31:04Z

campari/campari_runner.py

+            if self.create_ltcv_provenance:
+                raise NotImplementedError("Creating lightcurve provenance is not yet implemented in Campari.")
+            else:
+                assert self.ltcv_provenance_id is not None or \


As a general style thing -- rather than assert outside of tests, I prefer (and I think MWV also prefers) doing something like:

if not ( (self.ltcv_provenance_id is not None) or (self.ltcv_provenance_tag is not None and self.ltcv_process is not None) ): raise ValueError( "Must provide..." )

...in fact, it's more than a stylistic preference. You should not use assert outside of tests as a substitute for if plus raise, because it semantically isn't necessarily the same thing.

If somebody runs python with -O, all the assert statements are ignored!

Sometimes, rarely, it's useful inside production code, e.g. deep inside a loop where when in development you want to check a thing, but in production you don't want to take the performance hit of the check being run over and over again. However, that consideration does not apply to things that will be hit once, like verifying command-line arguments.

So make yourself an issue "get rid of all asserts outside of tests".

Got it, and issue raised!

rknop · 2025-10-30T13:39:10Z

campari/campari_runner.py

+                           f"process={self.diaobject_process}")
+
+            if self.diaobject_collection == "manual":
+                diaobjs = DiaObject.find_objects(collection=self.diaobject_collection, dbclient=self.dbclient,


Put a comment here referring to a snappl issue...

...and make a snappl issue that says we need to make DiaObject.find_object work for all collections, manual and otherwise, with the same call sequence. (That may be true already.)

The goal is, as I say a lot, to get campari not to ever look at the value of a collection variable.

I think that was the goal of issue #93 that I raised, now that that's fixed I'll delete this if statement.

rknop · 2025-10-30T13:40:11Z

campari/campari_runner.py

                            source="OpenUniverse2024"))

-        elif self.object_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):
+        elif self.diaobject_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):


As above, add a comment here referring to the snappl issue.

Correct me if I am wrong, but I think this one is fine? Here what I am doing is ensuring that if we are not using a manual collection that a SNID is passed, because a SNID (or other identifier) is needed to fetch an object from the database or ou2024. Or are you saying I should remove this and let snappl handle the failure?

Don't try to fix it now, but, yes, eventually snappl needs to handle the failure. If campari is looking at the value of collection at all, either it's doing something wrong, or snappl doesn't support what we need.

...do add the comment referencing the issue, however.

Actually, never mind, if you look at my other comments, this line is going to get deleted anyway.

rknop · 2025-10-30T13:41:36Z

campari/campari_runner.py

    def build_and_save_lightcurve(self, diaobj, lc_model, param_grid_row):
+
+        lc_model.image_collection_prov = self.img_coll_prov if self.use_real_images else None
        if self.psfclass == "ou24PSF" or self.psfclass == "ou24PSF_slow":


Do we already have an issue here about making it so campari doesn't look at the type of psf?

Yes this is Issue #183

rknop · 2025-10-30T13:45:01Z

campari/campari_runner.py

        if lc_model.flux is not None:
-            output_dir = pathlib.Path(self.cfg.value("photometry.campari.paths.output_dir"))
-            save_lightcurve(lc=lc, identifier=identifier, psftype=psftype, output_path=output_dir)
+            output_dir = pathlib.Path(self.cfg.value("system.paths.lightcurves"))


The condition for this should be save_to_db.

If you're not saving to db, then you should not write to system.paths.lightcurves, but somewhere else.

(Write to system.paths.lightcurves if and only if saving to db.)

Done, but, sorry, before you told me to switch to using system.paths.lightcurves (see above) should I switch those back?

Don't just switch; do the check on save_to_db. Never write to system.paths.lightcurves when not saving to db, always write there (and let snappl choose the file path) when saving to db

Note that if you are saving to db, you can pass None as the base dir to snappl; it will fill in the right thing itself

I assume tests should not be writing to the database?

rknop · 2025-10-30T13:46:30Z

campari/io.py

+            data_dict["NEA"].append(0.0)  # placeholder for now XXX TODO
+
+    meta_dict["band"] = band  # I don't ever expect campari to do multi-band fitting so just store the one band.
+    meta_dict["diaobject_position_id"] = None  # placeholder for now XXX TODO


Is there an issue for this?

Issue raised!

rknop · 2025-10-30T13:46:52Z

campari/io.py

+    meta_dict["diaobject_position_id"] = None  # placeholder for now XXX TODO
+    meta_dict["provenance_id"] = cam_prov.id
+    meta_dict["diaobject_id"] = diaobj.id
+    meta_dict["iau_name"] = diaobj.name  # I am not sure this is what IAUname is but it's a placeholder for now.


diaobj.iauname is a thing, just use that.

rknop · 2025-10-30T13:48:14Z

campari/io.py

+    meta_dict["provenance_id"] = cam_prov.id
+    meta_dict["diaobject_id"] = diaobj.id
+    meta_dict["iau_name"] = diaobj.name  # I am not sure this is what IAUname is but it's a placeholder for now.
+    meta_dict["ra_err"] = 0.0


THese three can be fixed in the same issue as diaobject_position_id

rknop · 2025-10-30T13:48:44Z

campari/io.py

-    lc.write(lc_file, format="ascii.ecsv", overwrite=overwrite)
+    band = lc.meta["band"]
+    SNLogger.debug(f"saving lightcurve for id={identifier}, band={band}, psftype={psftype}")
+    base_output_path = Config.get().value("system.paths.lightcurves") if output_path is None else output_path


As above : you should only use system.paths.lightcurves if save_to_database is True.

(In fact, also, if save_to_database is true and output_path is not None, then you should raise an exception.)

ColeFMeldorf · 2025-10-31T13:06:10Z

I believe lines 191-197 in campari_runner.py resolve the above issue but correct me if I am wrong.

rknop

OK, still pretty big changes. The biggest issue is one I should have noticed in previous PRs, but didn't. (Maybe I needed other stuff out of the way before I could see it?) To be able to work in Nov, you need to be able to take diaobject_id as the specifier for the SN, but you still only take name (though it's often called SNID as a throwback to terminology we used during the ou2024 days). As I've said a few times, name is not a reliable property in the database; it may well have duplicates, and may well be None for a bunch of objects.

Making this change would affect a lot of the code, so to simplify it, I have some comments that suggest you remove the whole business of being able to run on several supernovae in one run of campari. Just run on one supernova at a time. That lets you get rid of the vast majority of decide_run_mode. (Also, I probably didn't tag it, but you can remove the --SNID-file parameter.) Once you've taken all that out, the changes are not so severe.

rknop · 2025-10-31T19:58:05Z

campari/campari_runner.py

                            source="OpenUniverse2024"))

-        elif self.object_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):
+        elif self.diaobject_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):


...do add the comment referencing the issue, however.

rknop · 2025-10-31T20:08:11Z

campari/campari_runner.py

+            diaobjs = DiaObject.find_objects(collection=self.diaobject_collection, dbclient=self.dbclient,
+                                             provenance_tag=self.diaobject_provenance_tag,
+                                             process=self.diaobject_process, name=ID,
+                                             ra=self.ra, dec=self.dec, mjd_discovery_min=self.transient_start,
+                                             mjd_discovery_max=self.transient_end)


There's still the problem of the name/ID conflation going on here, which at the very least is confusing.
I realize this comes from the old school ou2024 calling stuff SNID. I would be very careful only to use SNID, and never just ID or Id or id... but even then I would make more changes (see below). But, there is a bigger problem, that I'm sorry I didn't notice before. This won't work for November. Right now, the only way to get campari to run on an object is to give it a name (which you call "SNID"). For November, you need to be able to pass in a diaobject_id. You do have a --diaobject-id command-line argument, but as far as I can tell you never actually look at what the user gave you, so that command-line argument is just ignored.

Here's what I would do:

Rename the -s, --SNID command-line argument to --diaobject-name. Let's just get away from using ID for things that aren't the database-defined diaobject_id.

Get rid of the --SNID-file command-line argument; I don't think there's any need to support a single run of campari running on lots of supernovae, because one supernova will take long enough that the overhead of starting a new campari run for each supernova is insignificant.

Rip out most of decide_run_mode ; only keep the bits at the end starting if self.img_list is not None. There is no need to think so hard about what the user gave you for the object. Also get rid of self.run_mode. If the user doesn't give enough information for some diaobject collection (e.g. manual), snappl will raise the necessary error already when you try to get the DiaObject.

Remove the loop that this call here is part of. (This is part of no longer supporting running on lots of objects in one run.) Move all the contents of the loop back one indent level.

In this call here, pass name=self.diaobject_name and diaobject_id=self.diaobject_id. (Assuming you changed the self.SNID to self.diaobject_name earlier, and that you pulled the --diaobject-id argument and put it in self.diaobject_id.

At that point, it should work, I think. If you want to run on an old-school ou2024 object, you pass a --diaobject-name (giving it the SNID from ou2024). If you want to run on the databsae, you might be able to give it a a --diaobject-name, but much more likely you'll give it a --diaobject-id. You may need to make sure any other places using self.SNID use self.name, and of course anywhere you run you'll have to change the command-line argumets you run with.

I take that this means I should get rid of --healpix and --nside as well?

rknop · 2025-10-31T20:13:53Z

campari/campari_runner.py

                            source="OpenUniverse2024"))

-        elif self.object_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):
+        elif self.diaobject_collection != "manual" and (self.SNID is None) and (self.SNID_file is None):


Actually, never mind, if you look at my other comments, this line is going to get deleted anyway.

rknop · 2025-10-31T20:14:39Z

campari/campari_runner.py

                diaobj.mjd_end = self.transient_end

-            SNLogger.debug(f"Object info for SN {ID} in collection {self.object_collection}: ra={diaobj.ra},"
+            SNLogger.debug(f"Object info for SN {ID} in collection {self.diaobject_collection}: ra={diaobj.ra},"


update this debug statement to use diaobject_id and name as described above (getting away from using ID for things that aren't the id.)

rknop · 2025-10-31T20:18:35Z

campari/campari_runner.py

            else:
-                identifier = self.run_name + "_" + str(diaobj.id)
+                identifier = self.run_name + "_" + str(diaobj.id if diaobj.id is not None else diaobj.name)
            if lc_model.flux is not None:


Put in a comment to that effect.

rknop · 2025-10-31T20:21:00Z

campari/campari_runner.py

            self.build_and_save_lightcurve(diaobj, lightcurve_model, param_grid_row)

    def decide_run_mode(self):
        """Decide which run mode to use based on the input configuration."""


Delete the entire contents of this function down to if self.img_list is not None:

It's not needed, if you also get rid of the other things I mentioned in the other suggested change above. That will save you from having to do all kinds of stuff here to think about name (renamed SNID) and diaobject_id... none of it's needed, because when you do DiaObject.find_objects, if the user hasn't given enough information, snappl will raise any necesary errors. (E.g. if the user asks for diaobject collection manual but doesn't give an ra and a dec, snappl will already yell; so, you don't have to check for that.)

rknop · 2025-10-31T20:21:51Z

campari/data_construction.py

        - detected: whether the exposure contains a detection or not.
    """
    SNLogger.debug(f"Finding all exposures for diaobj {diaobj.mjd_start, diaobj.mjd_end, diaobj.ra, diaobj.dec}")
+    SNLogger.debug(f"Using image source: {image_collection}")


Suggested change

SNLogger.debug(f"Using image source: {image_collection}")

SNLogger.debug(f"Using image collection: {image_collection}")

rknop · 2025-10-31T20:24:00Z

campari/data_construction.py

+    img_collection_prov = getattr(img_collection, "provenance", None)
+    if img_collection_prov is None:
+        SNLogger.warning("Image collection has no provenance information. This is only a problem if using snpitdb.")


I think you can delete these; by construction, snappl will never make a snpitdb image collection that doesn't have a Provenance. Check it out:

class ImageCollectionDB: ... def __init__( self, provenance=None, base_path=None ): if provenance is None: raise ValueError( "provenance is required" ) self.provenance = provenance

...actually, you return img_collection_prov, so don't delete that line. But you can delete the check.

rknop · 2025-10-31T20:27:09Z

campari/RomanASP.py

+                        help="A string to identify the process of the "
+                        "diaobject. Default None.", default=None)
+    parser.add_argument("--diaobject-id", type=str, default=None,
+                        help="the diaobject id. Default None.")  # Campari currently does not use this?


camapri needs to use it; see comment above.

It does now, and this comment has been removed.

rknop · 2025-10-31T20:29:07Z

examples/perlmutter/campari_config_test.yaml

    save_debug: true
    psfclass: ou24PSF

    paths:


Get rid of this whole section. All absolute paths need to be under system somewhere.

rknop · 2025-11-01T16:51:41Z

Yes

…

On November 1, 2025 9:49:07 AM PDT, Cole Meldorf ***@***.***> wrote: @ColeFMeldorf commented on this pull request. > + diaobjs = DiaObject.find_objects(collection=self.diaobject_collection, dbclient=self.dbclient, + provenance_tag=self.diaobject_provenance_tag, + process=self.diaobject_process, name=ID, + ra=self.ra, dec=self.dec, mjd_discovery_min=self.transient_start, + mjd_discovery_max=self.transient_end) I take that this means I should get rid of --healpix and --nside as well? -- Reply to this email directly or view it on GitHub: #195 (comment) You are receiving this because your review was requested. Message ID: ***@***.***>

ColeFMeldorf · 2025-11-01T17:26:50Z

I am also going to get rid of all of the simulation param_grid stuff too then, since that relied on the code being inside a for loop which I have now removed. Or at least, I'll move it for now and perhaps turn it into an external program I can run on campari sans loop. (Also you don't have to reply on weekends I am just using the comments section as a place to put notes.)

…

On Sat, Nov 1, 2025 at 12:52 PM Rob Knop ***@***.***> wrote: *rknop* left a comment (Roman-Supernova-PIT/campari#195) <#195 (comment)> Yes On November 1, 2025 9:49:07 AM PDT, Cole Meldorf ***@***.***> wrote: ***@***.*** commented on this pull request. > > > >> + diaobjs = DiaObject.find_objects(collection=self.diaobject_collection, dbclient=self.dbclient, >+ provenance_tag=self.diaobject_provenance_tag, >+ process=self.diaobject_process, name=ID, >+ ra=self.ra, dec=self.dec, mjd_discovery_min=self.transient_start, >+ mjd_discovery_max=self.transient_end) > >I take that this means I should get rid of --healpix and --nside as well? > >-- >Reply to this email directly or view it on GitHub: > #195 (comment) >You are receiving this because your review was requested. > >Message ID: ***@***.***> — Reply to this email directly, view it on GitHub <#195 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOIS2EPROQFPOMXVPQ75WLT32TQLHAVCNFSM6AAAAACKLROGDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINZWGU2TGNZVGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Co-authored-by: Rob Knop <[email protected]>

…campari into provenances

ColeFMeldorf · 2025-11-02T16:08:48Z

All comments addressed and all tests passing.

rknop

Getting closer. There are some log outputs that need to be fixed, plus I think there's still a case of something in there that assumes you're looping over a list of objects, even though that loop is now gone.

rknop · 2025-11-03T21:48:43Z

campari/campari_runner.py

+        # ra=self.ra, dec=self.dec
+        #    mjd_discovery_min=self.transient_start, mjd_discovery_max=self.transient_end
+
+        SNLogger.debug(f"Searching for DiaObject with id={self.diaobject_name}, ra={self.ra}, dec={self.dec},"


id={self.diaobject_id}, name={self.diaobject_name}

Really don't want to mix up id and name.

rknop · 2025-11-03T21:49:12Z

campari/campari_runner.py

+                                         mjd_discovery_max=self.transient_end)
+
+        if len(diaobjs) == 0:
+            raise ValueError(f"Could not find DiaObject with id={self.diaobject_name}, ra={self.ra}, dec={self.dec}.")


id={self.diaobject_id}, name={self.diaobject_name}

Really don't want to mix up id and name.

rknop · 2025-11-03T21:49:22Z

campari/campari_runner.py

+        if len(diaobjs) == 0:
+            raise ValueError(f"Could not find DiaObject with id={self.diaobject_name}, ra={self.ra}, dec={self.dec}.")
+        if len(diaobjs) > 1:
+            raise ValueError(f"Found multiple DiaObject with id={self.diaobject_name}, ra={self.ra}, dec={self.dec}.")


id={self.diaobject_id}, name={self.diaobject_name}

Really don't want to mix up id and name.

:(( Yeah I really missed all these, good catch

rknop · 2025-11-03T21:50:54Z

campari/campari_runner.py

+            diaobj.ra = diaobj.ra
+            diaobj.dec = diaobj.dec
+        else:
+            diaobj.ra, diaobj.dec = diaobj.get_position(provenance_tag=self.diaobject_position_provenance_tag,


Make it an error for self.ra or self.dec to either be non-None when you diaobject_position_provenance_tag is non-None.

If the user is specifying a position, then the provenance of the position is not the provenance given by that provenance tag.

rknop · 2025-11-03T21:52:29Z

campari/campari_runner.py

+                SNLogger.warning(f"Given Dec {self.dec} is far from DiaObject nominal Dec {diaobj.dec}")
+            diaobj.dec = self.dec
+
+        if (self.transient_start is not None):


Make in issue ...somewhere... that we need to think about how we're going to handle the whole notion of transient start and end.

Everything you've done so far has been easy because you have truth tables.

It becomes ill-defined otherwise. If it's always passed on the command line, then that becomes a little scary. We need to have a better procedure for deciding these in bulk. This isn't just a campari issue, this is a broarder Photometry issue.

rknop · 2025-11-03T21:57:55Z

campari/campari_runner.py

        SNLogger.debug("Created a grid of simulation parameters with a total of"
                       f" {self.param_grid.shape[1]} combinations.")
-        self.SNID = self.SNID * self.param_grid.shape[1]  # Repeat the SNID for each combination of parameters
+        self.diaobject_name = self.diaobject_name * self.param_grid.shape[1]  # Repeat the SNID for each combination of parameters


I don't think this makes sense now that you no longer have a for loop over objects.

When I told you to take that for loop out, I wasn't really thinking about your param grid; sorry about messing all that up. I'm hoping that it's not too painful to make the param grid something external where you an generate a bunch of campari run statments. (Or, heck, a bunch of jobs you can submit to the queue all at once and do them in parallel.) I think it was going to be painful to deal properly with diaobject_id in all of the decide run mode and all of that. If it's not too ugly to make the param grid external to the main body of campari, I think that's cleaner. (It's cleanear as much as possible to separate out the simulating of images and the running of the code. If it's all mixed together, it's harder to be sure you're running the same code on simulations as on actual data. When the simulations are all created externally and run through literally the same code, you can be more sure.) (Refactoring how you handle sims is for a future PR, not this one. For this one, just make sure you don't have self-inconsistencies.)

Make sure that your param grid stuff is consistent. If self.diobject_name is treated as a string, and not a list you loop over, then don't make it a list here, as that will break the code in other places.

Yes my plan is to make it external! I think that's the cleanest way to do it. It also may look a little different depending on how advanced we make your image simulator. In the spirit of that, I think for now I'll comment out this code and reintroduce it later. If you prefer the commented code gone just let me know.

Provenances f26e0fd

root added 2 commits October 24, 2025 22:21

Almost done

f0e06d3

All Tests Passing

4670466

github-actions bot added the testing label Oct 27, 2025

towncrier

b7d84ff

ColeFMeldorf requested review from rknop and wmwv October 27, 2025 19:46

ColeFMeldorf marked this pull request as ready for review October 27, 2025 19:46

rknop requested changes Oct 28, 2025

View reviewed changes

ColeFMeldorf mentioned this pull request Oct 29, 2025

Fetch object positions from the database #196

Closed

Comments Addressed

4fad542

rknop requested changes Oct 29, 2025

View reviewed changes

ColeFMeldorf mentioned this pull request Oct 29, 2025

How do we deal with campari versioning? #198

Open

comments addressed

ac85428

rknop reviewed Oct 30, 2025

View reviewed changes

root added 2 commits October 30, 2025 16:06

Comments addressed, tests passing

ecbac86

system lc path changed or wrapped in if savetodb if statement

d9cc531

ColeFMeldorf linked an issue Oct 31, 2025 that may be closed by this pull request

Fetch object positions from the database #196

Closed

ColeFMeldorf linked an issue Oct 31, 2025 that may be closed by this pull request

Use snappl lightcurve writing #186

Closed

ColeFMeldorf added Current Highest Priority for Merge sayeth Cole enhancement New feature or request and removed testing labels Oct 31, 2025

added database tests

ec5242e

rknop requested changes Oct 31, 2025

View reviewed changes

Update campari/campari_runner.py

e50e67e

Co-authored-by: Rob Knop <[email protected]>

ColeFMeldorf added 2 commits November 2, 2025 07:58

All Tests passing

2785438

Merge branch 'provenances' of https://github.com/Roman-Supernova-PIT/…

d1b1c3e

…campari into provenances

rknop requested changes Nov 3, 2025

View reviewed changes

ColeFMeldorf mentioned this pull request Nov 4, 2025

How should campari/photom handle transient_start and end? #204

Open

Comments addressed.

33d25d4

rknop approved these changes Nov 4, 2025

View reviewed changes

ColeFMeldorf merged commit f26e0fd into main Nov 4, 2025
2 checks passed

github-actions bot pushed a commit that referenced this pull request Nov 4, 2025

Merge pull request #195 from Roman-Supernova-PIT/provenances

fb38516

Provenances f26e0fd

		params=cfg, # keepkeys=["photometry.campari"],
		omitkeys=["photometry.campari.galsim", "photometry.campari.simulations"],

	SNLogger.debug(f"Using image source: {image_collection}")
	SNLogger.debug(f"Using image collection: {image_collection}")

Provenances #195

Provenances #195

Uh oh!

Conversation

ColeFMeldorf commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rknop left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColeFMeldorf commented Oct 28, 2025

Uh oh!

ColeFMeldorf commented Oct 28, 2025

Uh oh!

rknop left a comment

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!

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!

ColeFMeldorf commented Oct 27, 2025 •

edited

Loading