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

DSA-110 FRB photometry from Sharma et al (2024) #193

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

caseyjlaw
Copy link
Collaborator

The missing values are negative, but not exactly the same as used by others.
Let me know what you think.

@caseyjlaw
Copy link
Collaborator Author

Anything I can do to advance this? My plan was to get this merged and then work on the hosts.

@profxj
Copy link
Contributor

profxj commented Dec 4, 2024

Thanks for this, @caseyjlaw !

Any chance you can make the bad values -999. instead of a range of negative values?

@profxj
Copy link
Contributor

profxj commented Dec 4, 2024

oh, and the filters will need to match the schema described in here:

https://github.com/FRBs/FRB/blob/main/frb/galaxies/defs.py

and the magnitudes are assumed to not have been corrected for Galactic extinction.
The code will do that for you. Thanks!

@caseyjlaw
Copy link
Collaborator Author

I've modified the csv (and added a def) according to your suggestion. Do the CI tests cover validation of csv files or should I test another way?

@profxj
Copy link
Contributor

profxj commented Dec 6, 2024

Well, the next steps are to generate JSON files for the hosts which is probably
easiest for me to do.

But, it would be easier on me if this were a branch in the Repo (instead of grabbing your fork).

So, I suggest we redirect this PR into a branch
and then work on that branch together.

Good by you, @caseyjlaw ?

@profxj
Copy link
Contributor

profxj commented Dec 6, 2024

I should add, the JSON files are auto-generated, e.g.

frb_build Hosts --frb TNS

but there is a fair bit of public data downloading that goes on
which requires additional packages, etc.

@caseyjlaw
Copy link
Collaborator Author

Merging via a branch is fine by me. This PR is going from a new branch on my fork to a new branch on the main repo. We could just approve and merge into that new branch, then merge again into main after doing the work you suggested.

@profxj
Copy link
Contributor

profxj commented Dec 6, 2024

Ah, I had missed that! :)

Am approving and merging now. and we can issue a new PR from
that branch eventually.

@profxj profxj merged commit 0b5de52 into FRBs:dsa_frbs Dec 6, 2024
0 of 4 checks passed
@caseyjlaw
Copy link
Collaborator Author

Ok, just let me know how I can help.

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.

2 participants