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

First ingest wise_1810-1010 #427

Merged
merged 27 commits into from
Dec 13, 2023
Merged

Conversation

LishaRamon
Copy link
Collaborator

@LishaRamon LishaRamon commented Dec 5, 2023

Data of Wise_1810-1010 that are ingested in this script are publications, sources, names and parallaxes.

Link to relevant issue: Closes #258

For data ingests:

  • includes script used for ingest
  • includes modified JSON files
  • Add new tests
  • Update the Versions table

@LishaRamon
Copy link
Collaborator Author

Will be adding tests next.

@LishaRamon LishaRamon requested a review from kelle December 5, 2023 22:23
@kelle
Copy link
Collaborator

kelle commented Dec 5, 2023

This looks good but somehow you're also modifying 2mass_j12560183-1257276.json. That should not be part of this pull request.

@LishaRamon
Copy link
Collaborator Author

A .json file wasnt produced when saving the db, assuming because the code doesn't ingest yet. There is an error with source not being recognized as well.

@kelle
Copy link
Collaborator

kelle commented Dec 7, 2023

Good work! Other than the one bug with source, this looks great!

scripts/ingests/ingest_wise_1810-1010.py Outdated Show resolved Hide resolved
scripts/ingests/ingest_wise_1810-1010.py Outdated Show resolved Hide resolved
scripts/ingests/ingest_wise_1810-1010.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@LishaRamon LishaRamon left a comment

Choose a reason for hiding this comment

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

Viewed and implemented changes

@LishaRamon
Copy link
Collaborator Author

Trying to push current code, but saying I need to do a pull request first. Approved the changes you sent and implemented them. Used git fetch to make sure the files are the same. Not able to push at the moment, any suggestions?

@LishaRamon LishaRamon closed this Dec 8, 2023
@LishaRamon LishaRamon reopened this Dec 8, 2023
@LishaRamon
Copy link
Collaborator Author

I believe all of them pushed (showed up together). Most recent one (push attempt) should be the current one. One issue is there being a similar publication in the data. And occasionally says "add_publications" is not defined.

data/2mass_j12560183-1257276.json Outdated Show resolved Hide resolved
@LishaRamon
Copy link
Collaborator Author

Both commits have the same script, just got pushed twice on accident.

@kelle
Copy link
Collaborator

kelle commented Dec 11, 2023

The parameter names MUST be in the Parameters table. That's why you're getting foreign key constraint errors. "Radius" is not in the Parameters table.

@LishaRamon
Copy link
Collaborator Author

LishaRamon commented Dec 11, 2023

The json file you linked is "2MASS J12560215-1257217". But the file you wanted me to edit was "2MASS J12560183-1257276". Just checking if that's what you meant to send.

Effective temperature would be added as another parameter is that right? Since it's from the poster, what would the reference change to? Since we didnt ingest the poster

@kelle
Copy link
Collaborator

kelle commented Dec 11, 2023

Yes, you're right. I linked to the wrong file.

@kelle
Copy link
Collaborator

kelle commented Dec 11, 2023

And yes, effective temperature is another parameter.

@LishaRamon
Copy link
Collaborator Author

LishaRamon commented Dec 11, 2023

Since the value is less than or equal to 1000, would there be an appropriate error value to input?
How would we reference the poster in the db?

Corrected the right json file, appreciate the link, it was right above it :)

@kelle
Copy link
Collaborator

kelle commented Dec 11, 2023

Look at Table 6 from the paper that's linked in the issue. Do not use the poster values.

@LishaRamon
Copy link
Collaborator Author

The commit named "Jsons updated / t eff edit needed" with the number '[b6e5a08]" is the correct one with current changes. Others were repeated trial push attempts with the previous error. Just to make 4 file updates easier to see:

Versions.py- latest version was added/updated
2MASS J12560183-1257276.json- checked it was identical to the original db
CWISEP J181006.00-101001.1.json- under modeled parameters: mass(value_error changed to null and comments added/corrected), radius(unit changed to R_sun)
ingest_wise_1810-1010.py- Effective Temp parameters were added, checked, still does not show in db/json file

Comment on lines 561 to 563
ref = 'T eff'
t = db.query(db.ModeledParameters).filter(db.ModeledParameters.c.parameter == ref).astropy()
assert len(t) == 174, f'found {len(t)} modeled parameters with {ref} parameter'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ref = 'T eff'
t = db.query(db.ModeledParameters).filter(db.ModeledParameters.c.parameter == ref).astropy()
assert len(t) == 174, f'found {len(t)} modeled parameters with {ref} parameter'
param = 'T eff'
t = db.query(db.ModeledParameters).filter(db.ModeledParameters.c.parameter == param).astropy()
assert len(t) == 174, f'found {len(t)} modeled parameters with {param} parameter'

let's rename ref to be param so it's more readable/accurate.

@kelle
Copy link
Collaborator

kelle commented Dec 12, 2023

Good job getting the push to work!

I pushed a commit which removed the 1256 json file so you'll need to pull again.

did you save the database after adding the T_eff?

@kelle
Copy link
Collaborator

kelle commented Dec 12, 2023

T eff test looks great! Go ahead and add tests for the other parameters.

Copy link
Collaborator

@kelle kelle left a comment

Choose a reason for hiding this comment

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

looks great!!!

@kelle kelle merged commit 9f0aff0 into SIMPLE-AstroDB:main Dec 13, 2023
1 check passed
@LishaRamon LishaRamon deleted the Ingest-Wise-1810 branch December 13, 2023 16:47
@kelle kelle added this to the 2023.4 milestone Dec 13, 2023
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.

Ingest Wise 1810-1010, metal poor T dwarf
2 participants