-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
Will be adding tests next. |
This looks good but somehow you're also modifying |
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. |
Good work! Other than the one bug with source, this looks great! |
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.
Viewed and implemented changes
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? |
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. |
Both commits have the same script, just got pushed twice on accident. |
The parameter names MUST be in the |
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 |
Yes, you're right. I linked to the wrong file. |
And yes, effective temperature is another parameter. |
Since the value is less than or equal to 1000, would there be an appropriate error value to input? Corrected the right json file, appreciate the link, it was right above it :) |
Look at Table 6 from the paper that's linked in the issue. Do not use the poster values. |
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 |
tests/test_data.py
Outdated
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' |
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.
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.
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? |
T eff test looks great! Go ahead and add tests for the other parameters. |
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.
looks great!!!
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: