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

Repetition #43

Merged
merged 26 commits into from
Oct 20, 2023
Merged

Repetition #43

merged 26 commits into from
Oct 20, 2023

Conversation

cwjames1983
Copy link
Collaborator

This is a pretty large PR for FRB repetition behaviour!

It's composed of four components:

  • the general method, encapsulated in repeat_grid.py;
  • Miscellaneous additions/fixes to the main codebase
  • Specific work for modelling CHIME repetition in /papers/repeaters
  • Some extra work for FRB 210912

The current major issue with this is that it uses the old survey files, since I began this before the survey class was updated.

Hence, what I propose is to accept this PR, merge it with the main code, then I get a new clean checkout, and update all analyses to the new format (this won't be done urgently however, so maybe I just rename everything "old"?)

@profxj profxj self-requested a review June 28, 2023 17:32
papers/Repeaters/BeamData/all_freq_beam_10_bins.npy Outdated Show resolved Hide resolved
@@ -0,0 +1,98 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this code in Obsolete need
not be reviewed. But let me know if that's
not right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is true. Obsolete is for files which may in theory be useful, so I don't want them deleted; but they have no direct relevance to the analysis, so they're kept out of the way.

papers/Repeaters/Surveys/CHIME_150_fbar.dat Outdated Show resolved Hide resolved
import matplotlib
import time
from zdm import beams
beams.beams_path = '/Users/cjames/CRAFT/FRB_library/Git/H0paper/papers/Repeaters/BeamData/'
Copy link
Contributor

Choose a reason for hiding this comment

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

here and throughout, can we avoid hard-coding paths?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I thought I had removed all of these, but clearly not! All done now. Actually, most of this was obsolete stuff I just hadn't deleted...

@@ -0,0 +1,446 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

please move this under zdm, e.g. in a folder named chime

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment for any of the code in this current folder that would be needed by others to do "simple" CHIME analyses

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not needed for simple CHIME analysis - originally, yes it was, but not anymore! See file zdm/scripts/load_chime_data.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i.e. the file is staying where it is :-)

p2no_reps = np.exp(-N2reps)


def poisson_z(Robs,*args):
Copy link
Contributor

Choose a reason for hiding this comment

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

move this code elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to the recycle bin - I'd honestly forgotten this was in there, it's a relic from a first attempt at an analytic calculation. So deleted.




def calc_p_rep(Cr,V):
Copy link
Contributor

Choose a reason for hiding this comment

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

move this code elsewhere?

e.g. repeater_utils.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also legacy code, now deleted

@@ -0,0 +1,191 @@
"""
This script creates zdm grids and plots localised FRBs
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this doc is incorrect. Please update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, indeed it is - I'd forgotten about this, this file is now updated in several ways

zdm/survey.py Outdated
if self.NFRB==0:
raise ValueError('No FRBs found in file '+filename) #change this?

#if self.NFRB==0:
Copy link
Contributor

Choose a reason for hiding this comment

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

rm this code

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, this is OldSurvey

we should not be using this anymore, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, we shouldn't be using it. Maybe we shortly shift this to a separate "old_survey.py" file, to make development easier? However, I had to change this, since it's vital to allow surveys to have no FRBs in them. So I'll keep this code there, but we can discuss what to do about old_survey. I think we need to keep the code for posterity, but maybe shift it somewhere? Or I guess, this is what version control is for. Once we are 100% sure we have converted over all old .dat files, then we delete

zdm/survey.py Outdated
@@ -32,7 +32,6 @@ class Survey:

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized here that this branch has not pulled in main yet,
so I am going to stop reviewing until we do.

@profxj
Copy link
Contributor

profxj commented Jul 8, 2023

@cwjames1983 -- I was aiming to generate p(z|DM) for CHIME on this branch
so started on the README.txt file in papers/Repeaters, e.g. this step

################## dm_bias_fit.py  ####################

This produces a fit to the DM bias from CHIME Cat1.
Output is placed in "DMBiasFit/". The main figure
produced is "chime_bias_fit.py.

The key output of this has already been copied
to "survey.py" in /zdm/ to model the response of
the CHIME experiment.

This script also produces Figure 4 of the paper.

But the file chime_dm_bias.dat does not appear
to have been checked in to data/Misc (maybe call
that folder CHIME instead?). Please do so when
you have a chance. thx

from zdm import beams


beams.beams_path = '/Users/cjames/CRAFT/FRB_library/Git/H0paper/papers/Repeaters/BeamData/'
Copy link
Contributor

Choose a reason for hiding this comment

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

are these the same files as in zdm/papers/Repeaters/BeamData/ ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, and this is an old pathname, now deprecated and removed. Files now shifted to zdm/data/BeamData/CHIME/

zdm/iteration.py Outdated
const *= factor
return const

def OldConvertToMeaningfulConstant(pset):
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed!


self.calc_Rthresh(Exact=Exact,MC=MC,doplots=doplots)

def update(self,Rmin = None,Rmax = None,Rgamma = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

define Rmin, Rmax, Rgamma in doc string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

return Rc,Ntot


def calc_Rthresh(self,Exact=True,MC=False,Pthresh=0.01,doplots=False,old=[False,False,False]):
Copy link
Contributor

Choose a reason for hiding this comment

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

explain what Exact does

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

from zdm import survey
from matplotlib import pyplot as plt

def main():
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this script does much of anything.
Remove this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct - I've now updated it to be useful

@@ -88,7 +84,8 @@ def get_efficiency_from_wlist(self,DMlist,wlist,plist,
"""
efficiencies=np.zeros([wlist.size,DMlist.size])
if addGalacticDM:
toAdd = self.DMhalo + np.mean(self.DMGs)
# the following is safe against surveys with zero FRBs
Copy link
Contributor

Choose a reason for hiding this comment

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

How many of these changes need to propogate to the new Survey class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now fully debugged the new survey class to be robust against surveys with 0 and >0 FRBs. The methods are quite different in the new survey class.

Copy link
Contributor

@profxj profxj left a comment

Choose a reason for hiding this comment

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

Ok, finished a careful and comprehensive pass

@@ -1,7 +1,14 @@
"""
This script creates zdm grids and plots localised FRBs
This script shows how to use repeating FRB grids.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried running this script, but it fails right away.

@@ -5,6 +5,9 @@
It simply loads in CHIME data and makes sure
everything can be read correctly.

It also demonstrates how to generate "repeat grids"
Copy link
Contributor

Choose a reason for hiding this comment

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

I commented out 2 items that weren't used (and were causing my run to crash)

Copy link
Contributor

Choose a reason for hiding this comment

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

after that, 3 lovely plots were generated in a CHIME/ folder! :)

Copy link
Contributor

@profxj profxj left a comment

Choose a reason for hiding this comment

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

got the load_chime_data.py script to run successfully
with sensible looking figures generated.

I'm convinced this is ready

@cwjames1983 cwjames1983 merged commit a28d973 into main Oct 20, 2023
6 checks passed
@profxj profxj deleted the repetition branch October 21, 2024 14:56
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