-
Notifications
You must be signed in to change notification settings - Fork 25
Ingest Calamari Samples #631
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
Ingest Calamari Samples #631
Conversation
I need the ra and dec data for the primary
…004/SIMPLE-db into ingest_calamari_sample
|
Looks super great! I need to sit down and give it a more careful review but script and JSONs look excellent. |
kelle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments, one big one: primary stars do not need to be ingested.
data/reference/Publications.json
Outdated
| "description": "SCExAO/CHARIS Direct Imaging Discovery of a 20 au Separation, Low-mass Ratio Brown Dwarf Companion to an Accelerating Sun-like Star" | ||
| }, | ||
| { | ||
| "reference": "Gaia21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already ingested as GaiaEDR3
data/source/_47_oph.json
Outdated
| { | ||
| "Sources": [ | ||
| { | ||
| "source": "* 47 Oph", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try ingesting source without "*"
data/source/_eta_crb_ab.json
Outdated
| { | ||
| "Sources": [ | ||
| { | ||
| "source": "* eta CrB AB", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try ingesting source without "*"
data/source/ε_indi_a.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this file should be deleted.
| logger.info(f"references ingested:{ref_ingested}") # 10 references ingested | ||
| logger.info(f"references already exists:{ref_already_exists}") # 24 references due to preexisting data | ||
| logger.info(f"total references:{ref_ingested+ref_already_exists}") # 34 references total | ||
| logger.info(f"sources ingested:{sources_ingested}") # 42 ingested | ||
| logger.info(f"sources already exists:{sources_already_exists}") # 74 due to preexisting data | ||
| logger.info(f"total sources:{sources_ingested+sources_already_exists}") # 116 sources total | ||
| logger.info(f"companion relationships ingested:{companions_ingested}") # 101 ingested | ||
| logger.info(f"companion relationships already exists:{companions_already_exists}") # 15 due to preexisting data | ||
| logger.info(f"total companion relationships:{companions_ingested+companions_already_exists}") # 116 total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all of these make sense given the needed changes to the tests?
data/source/hd_33632.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this source ingested, just it's child.
data/source/hd_253662b.json
Outdated
| "other_name": "2MASS J06135342+1514062" | ||
| }, | ||
| { | ||
| "other_name": "HD 253662" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This in the parent, not another name for the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the scripts related to ingesting publications to their own script. We also want to think about how to best add ingesting from a URL capability to ingest_publication.
Co-authored-by: Kelle Cruz <kellecruz@gmail.com>
…004/SIMPLE-db into ingest_calamari_sample
|
When ingesting the primaries as the unresolved parent, it returns an error saying that the source doesn't exist. This is because I removed the ingest code for the primaries. Should I just not ingest these unresolved parents or should I write the code to ingest the primaries? I predict this issue will also pop up when ingesting the primaries as parents of the objects. Should I not ingest these relationships? |
I think all of the unresolved parents need to also be ingested as sources. This does not mean ingest all primaries, just the ones that are unresolved parents. Might be easiest to do this "by hand" or source-by-source. Also, I noticed that the "ingest_companion_relationships" function (simple/utils/companions.py) will need some modification to allow the new "unresolved child" relationship. |
Wait, I also think the issue is that some of the primary names are not SIMBAD resolvable. After searching the database, the function searches SIMBAD for the source's name. IF the name is unresolvable in SIMBAD, this returns no source found. |
|
Aha! Good find.
|
Ingest Emily Calamari's samples of UCDs with higher mass primaries. Sources, publications, companion relationships.
Link to relevant issue: Closes #633
Closes #636
For data ingests:
includes script used for ingest
includes modified JSON files
Add new tests
Update the Versions table