Third-party object types

[JoanneBogart]

Document use of third-party object types

[jchiang87]

There are a number of changes I'd like to see made.

[jchiang87]

My original motivations for adding this facility was somewhat different than the desire for an easy way to implement "small-scale catalogs", and I think are more compelling. So I would replace lines 3-12 with

The required interface for using catalogs with imSim just entails implementing a few methods in
subclasses of the ``BaseObject`` and ``ObjectCollection`` classes.  Thus, it's fairly straight-forward to
implement new object types, and we've provided an interface to enable those object types to imported
and configured via a third party package.  Using a third party package has a couple of notable benefits:

- It avoids having to coordinate adding new object types to skyCatalogs itself;
- Relatedly, any additional software dependencies introduced by the new code will be isolated from skyCatalogs and so won't impose those dependencies on other skyCatalogs users.

In addition, some of the supporting infrastructure in skyCatalogs, e.g., reading parquet files with
pre-computed fluxes, can in principle, be available via subclassing.

[JoanneBogart]

Most of your suggested text is fine with me. I'm not sure it's realistic for people to subclass anything other than perhaps ParquetReader (and of course BaseObject and ObjectCollection) so might rephrase the last paragraph.
We can leave out the mention of "small-scale catalogs", but I don't think we should be encouraging people to go the third-party route for large catalogs that others (not the developer) might want to use. The use of the interface might be appropriate, but the code should reside in the skyCatalogs and, optionally, skyCatalogs_creator repos.

[jchiang87]

Then I would just omit that last sentence from my text. Personally, I think skyCatalogs should be more of an interface package, and heavy duty implementation for specific catalogs should be spun off into separate packages. This helps keep the dependencies minimal, provides more stability for users, and would encourage the interfaces to be less specific to particular catalog implementations.

[JoanneBogart]

Happy to omit it.
I would agree with you about separate packages if skyCatalogs + skyCatalogs_creator had been designed that way, with more of the infrastructure readily subclassable. As it is, there is a substantial amount of code for creating and reading data files and dealing with the configuration which is shared by different object types.

[jchiang87]

I'd replace the sentence A realistic ... was created. with

If the object types have multiple components with distinct SEDs, such as bulge and disk components in
a galaxy, then the ``.subcomponents`` property for the ``BaseObject`` subclass must be defined to return a 
list of the subcomponents for each object.

[JoanneBogart]

I think we still need something from the original version in addition to your text. The point I was trying to make was that returning None, as is done in the example, is not realistic. But I agree we also need something about subcomponents.

[jchiang87]

In that case, I'd add this after my sentence about subcomponents:

Of course, contrary to the test code, the ``get_observer_sed_component`` function should return a
``galsim.SED`` object for the specified component.

[JoanneBogart]

I left my part first, which I think is easier to parse since it refers to the routines in the order they appear in the previous sentence, then included essentially your text.

[jchiang87]

I think this parenthetical statement is unnecessary:

(Alternatively one could write different code for the two object types in different modules.)

[JoanneBogart]

I don't see that it does any harm and there could be situations where that would be the better approach. What if the object types are different enough that the corresponding codes don't have much in common? And it's possible that someone, not necessarily the writer, is only interested in using one. Of course the code handling both would work perfectly well, but if the object types are unrelated and might be used independently or have different development cycles I'd say it's preferable to separate into two modules.

[jchiang87]

I think these lines are also unnecessary:

The minimum configuration needed would omit the second object type and
the ``object_param`` field from the remaining object type description.

[JoanneBogart]

I think it's useful for someone who otherwise has no information beyond what's in the example. They need to know what the fields are for, which are required and which are not.

[jchiang87]

In that case, I think it's better to state explicitly what's required rather than saying what can be omitted:

A minimal configuration would include just one object type and the area_partition and module fields, e.g.:

object_types:
  my_object_type:
    area_partition: None
    module: external_catalog

(BTW, it would be great if we could omit area_partition by setting it to None by default in the code.)

[jchiang87]

Add the sentence

For running with imSim, the ``.native_columns()`` property and ``.__len__()`` method must also be implemented.

[JoanneBogart]

There are implementations in ObjectCollection which look like they would do for your example.
Possibly related - in ExternalCollection.init why don't you do
super().__init__(...)
followed by
self._param = param
?

[jchiang87]

I'll note that code was intended for unit testing purposes, so I just implemented the parts I needed to test third party interface, not to test the full skyCatalogs interface. If you find it so objectionable as an example, then we shouldn't use test code and provide example code with the features you want.

[JoanneBogart]

I used it because it's all there was. I don't find it objectionable but there are features it, quite understandably, doesn't exercise. I took the easy way out by trying to fill in the gaps. It would be better to use an example specifically designed as a teaching tool. I can try my hand at it. I expect, as designer of the facility, you could do a better job.
Initially in my comment I was just trying to understand why you thought it necessary to tell people they needed to be concerned about .native_columns() and .__len()__ when the implementations in the base class would probably do for most people.