U/jrbogart/provenance v2 #111

JoanneBogart · 2024-06-21T19:17:07Z

Add provenance information to any created parquet files as metadata.
Include run options as part of provenance (both in parquet files and in output text config).
Also includes

bug fix so that, when SSO is not available, code responds gracefully
a change to handling of cosmology parameters to stay compatible with different versions of GCRCatalogs

Partially addresses issue #64

…lude non-standard cosmology parameters

…gs and astropy

jchiang87

I have some questions and change requests.

jchiang87 · 2024-07-09T16:06:38Z

skycatalogs/utils/sed_tools.py

@@ -220,13 +220,20 @@ def __init__(self, sed_path, fmt='db'):
        assumptions: that the table name is "SED" and that the columns are
        "wavelength" (units angstroms) and "flux" (units flambda)
        '''
+        if not sed_path:
+            # print a warning message?
+            return None


Is returning None really the right thing here? From the line above in skyCatalogs.py, if this object is returned as None, the sso object type simply isn't registered, but it seems like it should raise an exception if the needed SED info isn't available instead of silently omitting SSOs.

Right now the typical config file includes all object types supported by the code, whether or not they're actually available at the installation. This will change with the stuff I'm currently working on (which, among other things, should make it easier to include only those object types which are available), but for now the fact that the SSO SED isn't there is often the first sign the code sees that there are no SSOs. I could add a warning message but would rather not raise an exception. That would force people to meddle with the configs by hand (to remove the sso section) more than I'd like.

jchiang87 · 2024-07-09T16:07:46Z

skycatalogs/skyCatalogs.py

-                                              object_class=SsoObject,
-                                              collection_class=SsoCollection)
+            if self._sso_sed_factory:
+                self.cat_cxt.register_source_type('sso',


See my comment below in sed_tools.py. Silently omitting SSOs here seems wrong.

I'll come up with a suitable place to log a warning message.

jchiang87 · 2024-07-09T16:18:27Z

skycatalogs/catalog_creator.py

+                                               inputs=inputs,
+                                               run_options=self._run_options)
+
+        arrow_schema = make_star_schema(metadata_input=file_metadata)


Is the comment below,

# Need a way to indicate which object types to include; deal with that # later. For now, default is stars + sn

still relevant? I would think not, since sncosmo support has been removed. As it stands, it's hard to see how it relates to the code around it.

Nope, no longer relevant. Will delete.

jchiang87 · 2024-07-09T16:26:58Z

skycatalogs/catalog_creator.py

@@ -1165,5 +1193,6 @@ def write_provenance_file(self, datafile_path):
        '''
        outpath = datafile_path.rsplit('.', 1)[0] + '_provenance.yaml'

-        prov = assemble_provenance(self._pkg_root, inputs=None)
+        prov = assemble_provenance(self._pkg_root, inputs=None,


It seems inconsistent that we have inputs=None for this provenance and an inputs value that's not None above. Shouldn't the same provenance info be used in both places?

Not necessarily. In the first case, a global config file is being written. The value for inputs is all the possible inputs catalog_creator knows about. In the second, it's only associated with a particular binary file. Probably there should have been an inputs argument to write_provenance_file so that inputs appropriate to the object type of that file could be included. (That's what I've done in the code I'm working on where there is no global provenance, but only provenance associated with an object type) But, now that provenance is being included in the binary file (which includes only relevant inputs), I don't think we need this routine altogether. My inclination is to get rid of it and the option in the creation script which causes it to be called.

jchiang87 · 2024-07-09T16:38:57Z

skycatalogs/utils/common_utils.py

+def callinfo_to_dict(args):
+    """
+    Make a dict out of program arguments.  Each option value is
+    either a simple atomic type or a list
+    """
+    a_dict = dict()
+    for e in dir(args):
+        if not e.startswith('_'):
+            v = eval('args.' + e)
+            a_dict[e] = v
+    return a_dict
+


It seems that

dict(args._get_kwargs())

does the same thing as this function.

So it does. I could also use _get_kwargs to simplify log_callinfo but the results aren't alphabetized, which is useful in that case. Another similar routine,print_callinfo, isn't used so I will remove it.

To alphabetize by keyword,

dict(sorted(args._get_kwargs()))

would work.

Ok. I'll make that change.

jchiang87 · 2024-07-09T16:42:38Z

skycatalogs/catalog_creator.py

@@ -237,7 +238,8 @@ def __init__(self, parts, area_partition=None, skycatalog_root=None,
                 galaxy_stride=1000000, provenance=None,
                 dc2=False, sn_object_type='sncosmo', galaxy_type='cosmodc2',
                 include_roman_flux=False, star_input_fmt='sqlite',
-                 sso_truth=None, sso_sed=None, sso_partition='healpixel'):
+                 sso_truth=None, sso_sed=None, sso_partition='healpixel',
+                 run_options=None):


Can you add a docstring entry for run_options please?

Yes, certainly. Just an oversight.

jchiang87 · 2024-07-09T16:45:14Z

skycatalogs/catalog_creator.py

+        self._run_options = run_options
+
+        # Do we need this?
+        self._sed_bins = None


If this attribute were named something like self._cosmodc2_sed_bins or self._tophat_sed_bins, it would clearer in which contexts it's used. Based on the code below, setting to None here isn't strictly needed, but setting a private attribute in the __init__ seems like a good practice.

Agreed. I'll change the name.

…ng it to be invoked

JoanneBogart · 2024-07-10T21:23:02Z

CI check is failing because sdf is down, but nothing I did in this last commit (all in "create" part of skycatalogs) would affect it. This PR is ready for re-review.

jchiang87

Looks fine, but I would recommend making the warning more explicit about the outcome of the missing SSO files.

jchiang87 · 2024-07-11T16:03:23Z

skycatalogs/skyCatalogs.py

@@ -334,6 +334,9 @@ def __init__(self, config, mp=False, skycatalog_root=None, verbose=False,
                                                                    'sso_sed.db')

            self._sso_sed_factory = SsoSedFactory(self._sso_sed_path)
+            if not self._sso_sed_factory:
+                self._logger.warning('SSO appear in the list of available object types but supporting files do not exist')
+                self._logger.warning('SSOs will not be present in object lists')


I think that a warning that explicitly says that "SSOs will not be simulated" would be more helpful to users.

JoanneBogart added 5 commits June 19, 2024 17:43

1) include run options in provenance output. 2) Do not attempt to inc…

ff5b4a1

…lude non-standard cosmology parameters

exclude SSO when site does not support it (e.g. no SED file)

5544bed

include provenance information as metadata in parquet files

a5594ba

Ensure construction of cosmology works with all versions of GCRCatalo…

aa68872

…gs and astropy

eliminate commented-out code

45759a1

JoanneBogart requested a review from jchiang87 July 3, 2024 18:50

jchiang87 requested changes Jul 9, 2024

View reviewed changes

JoanneBogart added 2 commits July 10, 2024 12:09

address reviewer comments

5d19e22

remove unnecessary routine write_provenance_file and the option causi…

260f7fe

…ng it to be invoked

JoanneBogart requested a review from jchiang87 July 10, 2024 21:20

jchiang87 approved these changes Jul 11, 2024

View reviewed changes

improve warning message

d2276b7

JoanneBogart merged commit da8e100 into main Jul 11, 2024
1 check failed

JoanneBogart deleted the u/jrbogart/provenance_v2 branch July 11, 2024 18:07

JoanneBogart mentioned this pull request Jul 31, 2024

Better provenance tracking #64

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

U/jrbogart/provenance v2 #111

U/jrbogart/provenance v2 #111

JoanneBogart commented Jun 21, 2024 •

edited

Loading

jchiang87 left a comment

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024 •

edited

Loading

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024 •

edited

Loading

jchiang87 Jul 9, 2024

JoanneBogart Jul 10, 2024 •

edited

Loading

jchiang87 Jul 10, 2024

JoanneBogart Jul 10, 2024

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024

jchiang87 Jul 9, 2024

JoanneBogart Jul 9, 2024

JoanneBogart commented Jul 10, 2024 •

edited

Loading

jchiang87 left a comment

jchiang87 Jul 11, 2024

U/jrbogart/provenance v2 #111

U/jrbogart/provenance v2 #111

Conversation

JoanneBogart commented Jun 21, 2024 • edited Loading

jchiang87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanneBogart Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanneBogart Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanneBogart Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanneBogart commented Jul 10, 2024 • edited Loading

jchiang87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanneBogart commented Jun 21, 2024 •

edited

Loading

JoanneBogart Jul 9, 2024 •

edited

Loading

JoanneBogart Jul 9, 2024 •

edited

Loading

JoanneBogart Jul 10, 2024 •

edited

Loading

JoanneBogart commented Jul 10, 2024 •

edited

Loading