allow parallelization for star flux computation #85

JoanneBogart · 2024-02-16T23:42:37Z

Implemented essentially the same procedure used to parallelize galaxy flux computation.

jchiang87

I have a couple of suggestions and requests for clarification.

jchiang87 · 2024-05-14T22:21:31Z

skycatalogs/catalog_creator.py

+def _do_star_flux_chunk(send_conn, star_collection, instrument_needed,
+                        l_bnd, u_bnd):
+    '''
+    end_conn         output connection


end_conn -> send_conn. Can you be more explicit about what sort of "connection" this is?

Addressed in next commit.

jchiang87 · 2024-05-14T22:25:38Z

skycatalogs/catalog_creator.py

+                        l_bnd, u_bnd):
+    '''
+    end_conn         output connection
+    star_collection  information from main file


This is actually an ObjectCollection, isn't it? It would be helpful to note that here.

addressed in next commit

jchiang87 · 2024-05-14T22:29:11Z

skycatalogs/catalog_creator.py

+    '''
+    end_conn         output connection
+    star_collection  information from main file
+    instrument_needed List of which calculations should be done


Looks like the only relevant entries are 'lsst' and 'roman'. It would be good to note that here. Should there be a check somewhere that this list contains at least one of those two values?

I'll add something to the parameter description in the docstring.
There could be a check that instrument_needed has at least one valid value, but there is nothing like that level of checking generally on internal routines. The caller always includes 'lsst'.

jchiang87 · 2024-05-14T22:32:20Z

skycatalogs/catalog_creator.py

@@ -990,6 +1025,9 @@ def _create_pointsource_flux_pixel(self, pixel):
        # For schema use self._ps_flux_schema
        # output_template should be derived from value for flux_file_template
        #  in main catalog config.  Cheat for now
+
+        global _star_collection


Can you add a comment here why this needs to be global? Given that it's being passed as an argument to _do_star_flux_chunk, it wouldn't seem to need to be.

I'm not positive it does need to be global. The star code is following the same procedure as the galaxy code multiprocessing code, which was written a while ago. My intent then was to make the rather large _star_collection available to subprocesses without pickling. I don't know whether that is in fact the case.
If it's not I would do better to just pass in the slice of _star_collection that each subprocess needs, but that is not something I want to take on for this PR.

It seems like you would need to omit _star_collection from the argument list and declare it global inside of _do_star_flux_chunk to avoid having it pickled. Given that it's being passed as an argument, I think it must be being serialized, and the global declaration here isn't doing anything. If so, then there's no reason to pass l_bnd and u_bnd and the slicing can be done in the calling code.

that is not something I want to take on for this PR.

The code may work fine as-is, but these apparent inconsistencies seem like good reasons to try to understand what's going on in case there is something untoward actually happening. So if you don't want to change it, I'd suggest at least some test code to ensure it is behaving as expected.

I agree in principle; packaging this up in test code will take some thought. When I first implemented the identical scheme for galaxies I proceeded pretty carefully by

examining whatever I could in the debugger

comparing output from creating a flux file with number-of-subprocesses = 1 (in which case the code doesn't use subprocesses at all and just calls the _do_X_flux_chunk routine directly) and number-of-subprocesses set to something realistic, like 20. The outputs were identical.

I did the same thing when I implemented parallel processing for stars. I haven't done the comparison test for this last commit, but the code changes have nothing to do with the parallel processing part of the code.

Sure, but statements like this

My intent then was to make the rather large _star_collection available to subprocesses without pickling. I don't know whether that is in fact the case.

make it rather unclear to me, without actually having run the code (which apparently isn't that easy to do), that the unmodified implementation is correct. Regardless of whether consistent outputs are obtained with 1 or more than 1 subprocess, it would be nice to have the global vs not global aspects of the code make sense. Right now, they don't appear to me to be consistent. Given that it looks like global _star_collection isn't doing what's intended, it would be remiss of me not to point that out and suggest a fix.

jchiang87 · 2024-05-14T22:36:54Z

skycatalogs/catalog_creator.py

        object_list = self._cat.get_object_type_by_hp(pixel, 'star')
-        last_row_ix = len(object_list) - 1
-        writer = None
+        obj_coll = object_list.get_collections()[0]


Why can't this simply be

_star_collection = object_list.get_collections()[0]

?

No reason. Changed in next commit.

jchiang87 · 2024-05-14T22:46:22Z

skycatalogs/catalog_creator.py

+        else:
+            n_per = int((u_bnd - l_bnd + n_parallel)/n_parallel)
+        fields_needed = self._ps_flux_schema.names
+        instrument_needed = ['lsst']       # for now


It seems like instrument_needed should be set as a instance-level attribute and set in the .__init__(...), instead of hard-wired here.

Yes, it should ultimately comes from the way the outer script was called but currently the assumption is that lsst is always included and including roman is an option. I could see making that assumption explicit in the outer script and flowing only from there, but I think not in this PR.

jchiang87 · 2024-05-14T23:03:47Z

skycatalogs/catalog_creator.py

+        for i, band in enumerate(LSST_BANDS):
+            v = all_fluxes_transpose.__next__()
+            out_dict[f'lsst_flux_{band}'] = v


The explicit use of .__next__() seems a bit unwieldy. I think the following would be more conventional:

colnames = [f'lsst_flux_{band}' for band in LSST_BANDS] flux_dict = dict(zip(colnames, all_fluxes_transpose)) out_dict.update(flux_dict)

Changed in next commit

jchiang87 · 2024-05-14T23:14:02Z

skycatalogs/catalog_creator.py

+def _do_star_flux_chunk(send_conn, star_collection, instrument_needed,
+                        l_bnd, u_bnd):


I think it would be a bit cleaner to simplify this interface by doing the slicing in the calling code. So the new interface would effectively become:

def _do_star_flux_chunk(send_conn, o_list, instrument_needed):

and in the calling code, it would be used like this:

_do_star_flux_chunk(send_conn, star_collection[l_bnd: u_bnd], instrument_needed)

Yes, see comment above about global declaration.

allow parallelization for star flux computation

04c03bd

JoanneBogart requested a review from jchiang87 February 16, 2024 23:42

jchiang87 requested changes May 14, 2024

View reviewed changes

JoanneBogart added 3 commits May 14, 2024 19:29

address reviewer comments

98889c4

omit unnecessary global declarations

864634c

bug fix for Config.get_config_value

d0f7962

jchiang87 approved these changes May 16, 2024

View reviewed changes

JoanneBogart merged commit e6b60f3 into main May 16, 2024
1 check passed

JoanneBogart deleted the u/jrbogart/parallel_star branch May 20, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow parallelization for star flux computation #85

allow parallelization for star flux computation #85

JoanneBogart commented Feb 16, 2024

jchiang87 left a comment

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

jchiang87 May 14, 2024

JoanneBogart May 15, 2024 •

edited

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

jchiang87 May 15, 2024 •

edited

JoanneBogart May 15, 2024 •

edited

jchiang87 May 15, 2024

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

jchiang87 May 14, 2024 •

edited

JoanneBogart May 15, 2024

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

jchiang87 May 14, 2024

JoanneBogart May 15, 2024

		def _do_star_flux_chunk(send_conn, star_collection, instrument_needed,
		l_bnd, u_bnd):

allow parallelization for star flux computation #85

allow parallelization for star flux computation #85

Conversation

JoanneBogart commented Feb 16, 2024

jchiang87 left a 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 May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchiang87 May 15, 2024 • edited

Choose a reason for hiding this comment

JoanneBogart May 15, 2024 • edited

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

jchiang87 May 14, 2024 • edited

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 May 15, 2024 •

edited

jchiang87 May 15, 2024 •

edited

JoanneBogart May 15, 2024 •

edited

jchiang87 May 14, 2024 •

edited