Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tickets/DM-11283: Spec out DIAObjectCollection API #2

Merged
merged 3 commits into from Aug 1, 2017

Conversation

morriscb
Copy link
Contributor

No description provided.

----------
dia_objects : a list of DIAObjects
List of DIAObjects that represent the collection of variable
objects for this visit. Each DIAOjbect will be individually
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIAOjbect -> DIAObject

pass

def match(self, dia_source_catalog, scores):
""" Append DIAsources to DIAObjectds given a score and create new
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIAObjectds -> DIAObjects

Parameters
----------
dia_source_catalog : an lsst.afw.SourceCatalog
A contiguous catalo of dia_sources for which the set of scores
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catalo -> catalog

Parameters
----------
dia_source_catalog : an lsst.afw.SourceCatalog
A contiguous catalo of dia_sources to "score" based on distance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catalo -> catalog

Attributes
----------
dia_objects : a list of DIAObjects
List of DIAObjects representing this collection the current (e.g.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than storing the DIAObjects as a list, I think it may make sense to store them in a dictionary for fast access by id. I am open to counterarguments, though.

Copy link
Contributor Author

@morriscb morriscb Aug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. One hesitation I have for this is that the scoring and matching step is most easily done on the relative position in a list rather than a id in a dictionary. What if I add a dictionary internal to the class that is keyed on the id and returns the index in the list? I think I would hide this from user through some overwriting of built in functions or create a accessor method that uses the id as the input and returns the DIAObject of interest through the dictionary of id to list position. Let me know if this sounds reasonable and I'll implement it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just made a first past commit on a the dictionary I described. Let me know if this works for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Ship it.

bool
"""

return self._is_valid_tree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any methods dealing with persistence to/from a data store in here. Should those be internal to the class or external? My guess is external (mocking up something like Butler.put()), etc., but can you tell me your thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I think this is something that gets taken care of by the future AssociationTask. On instantiation this would query where ever the DIAObjects are stored and then create the DIAObjectCollection for the association step. AssociationTask would then take care of writing the data as well.

Added id_to_index dictionary and method

Finished review comments.
@morriscb morriscb merged commit c2bb45d into master Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants