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

Reader for matched-table catalog and config files for composite add-o… #393

Merged
merged 14 commits into from
Nov 6, 2019

Conversation

evevkovacs
Copy link
Contributor

@yymao @fjaviersanchez Here is a reader for the new table, created by Javi to find objects in the dc2 object catalog that have matches in cosmoDC2. This reader just reads in the fits file and generates some useful quantities. I should be able to run some descqa tests on this table/mini-catalog. I have requested both of you as reviwers. The next steps are 1)to make it an add-on to the dc2-object catalog, since people might want to check other quantities and 2) to make cosmoDC2 an add-on so other tests can be run. I have also pushed my first attempt at the config file for using the new reader as part of a composite catalog. It doesn't crash, but it is not giving me the result that I expected. The object catalog and the matched table both have 7548496 rows, but when I read in the composite catalog (which is matched on objectId) I get 78649132 rows, so I have not understood something about how composite catalogs work. Could one of you take possibly take a look and see if you can spot the problem?

fjaviersanchez
fjaviersanchez previously approved these changes Oct 9, 2019
Copy link
Contributor

@fjaviersanchez fjaviersanchez left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thanks for putting this together Eve!

@yymao
Copy link
Member

yymao commented Oct 9, 2019

Thanks @evevkovacs @fjaviersanchez --- about the questions @evevkovacs raised:

  1. to make it an add-on to the dc2-object catalog

This can work by either matching the object_id column like you did, or, if the object catalog and the matched table have the same row order, use the MATCHING_ORDER method. (It seems that the matched table is a single fits file, so we can't use theMATCHING_FORMAT method, since it requires the add-on catalog to have exactly the same order.)

In the composite config, it appears that you are matching to dc2_object_run2.1i_dr1 not dc2_object_run2.1.1i. This is probably what causes the issue?

  1. to make cosmoDC2 an add-on so other tests can be run

I now realized that this is a bit beyond the original design of the composite catalog, which was to add add-on columns to a main catalog by matching to a single column of the main catalog. Here, the cosmoDC2 needs to be matched to a different column (truth_id).

While this possible, the implementation would look quite ugly unless we go change the underlying composite catalog code. But before we further expand GCR's functionality, we should really ask if we should instead explore using database instead.

yymao
yymao previously requested changes Oct 9, 2019
GCRCatalogs/dc2_matched_table.py Outdated Show resolved Hide resolved
….yaml

Co-Authored-By: Yao-Yuan Mao <yymao.astro@gmail.com>
@@ -1,5 +1,7 @@
subclass_name: dc2_matched_table.DC2MatchedTable
filename: '/global/projecta/projectdirs/lsst/groups/SSim/DC2/matched_ids_run2.1.1i_dpdd.fits.gz'
Copy link

Choose a reason for hiding this comment

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

I propose that we use this pull request to fix the NERSC-specific paths problem. @yymao @boutigny @heather999 @wmwv

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to fully understand what code needs to change, see: #346
We also need to identify all of the catalogs and their current locations at NERSC. With a list, we can create an area at NERSC with a set prefix and agreed upon directory structure.

@evevkovacs
Copy link
Contributor Author

@yymao @fjaviersanchez The code is tested and works but fails the travis check with a warning I don't fully understand. The offending line is 127. The warning goes away if I add the argument "handle" to the call signature for the native_quantity_getter, but then the code crashes. Can one of you please take a look and see if you know how to make travis happy. Thanks

@evevkovacs
Copy link
Contributor Author

@fjaviersanchez @yymao I made one last attempt to fix the travis issue and this time it passed! So as soon as one of you approves, I can merge this code. Thanks a lot! I'd like to merge so that other people can run descqa tests if they want.

Copy link
Contributor

@fjaviersanchez fjaviersanchez left a comment

Choose a reason for hiding this comment

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

It looks good to me but I don't know if we want to put the matching scripts somewhere in the DC2-production repo or something like that.

@yymao yymao dismissed their stale review November 6, 2019 15:37

Currently on leave

@heather999
Copy link
Contributor

@fjaviersanchez Please go ahead and add the matching scripts to DC2-production via a PR and then we should organize a tag of DC2-production.

@evevkovacs evevkovacs merged commit f472b87 into master Nov 6, 2019
@evevkovacs evevkovacs deleted the dc2addon branch November 6, 2019 16:58
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.

5 participants