-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add marker options to ligo-skymap-plot. #5
Conversation
The following options are added that specify shape, color, edge color and size of the marker used with the --radec option that already existed: --marker to specify the symbol --marker-color for the color --marker-ecolor for the edge color (outside line of the marker) --marker-size for the size Defaults are implemented so that the behaviour does not change when NOT specifying any of these new options. I.e. by default, --radec plots a size 10 star ('*') marker in white with a black outline.
The following options are added in a separate PTA group in the parser (so that they are displayed separately in the --help message): --pta-file: use a file pulsars.csv to specify ra, dec locations of the PTA pulsars and (optionally) a third column with pulsar noise rms to scale the markers with --pta-size: Optional to only use the first "n" pulsars from the file --pta-marker: specify the marker symbol (default '*') --pta-color: the marker color (default white) --pta-ecolor: the marker edge color (default black) --pta-fix-marker-size: Do not scale the markers with noise rms, instead use a single size value for all.
Added options to mark the pulsars from a Pulsar Timing Array. The following options are added in a separate PTA group in the parser The --pta-file needs to be a csv file with the first two columns being Ra and Dec (in degrees). There either needs to be a third column with the noise rms level (in seconds) of each pulsar, OR the option --pta-fix-marker-size needs to be specified. In the first case, the marker size of each pulsar is scaled with the rms level as: 10 * (rms / 1.e-7)**(-0.4). This is purely an empirical scaling I found looks good for typical rms noise values. Pulsars with lower noise rms ("better" pulsars) are displayed bigger. One could easily add another option for an overall size scaling by replacing the 10 in this relation. |
@@ -65,6 +77,33 @@ def parser(): | |||
parser.add_argument( | |||
'input', metavar='INPUT.fits[.gz]', type=FileType('rb'), | |||
default='-', nargs='?', help='Input FITS file') | |||
|
|||
# separate display group for PTA related options | |||
pta_group = parser.add_argument_group('PTA options', |
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.
I am a little hesitant to support a new file type. Is this an established format that is documented somewhere?
Since your input is a CSV file, I wonder if you could accomplish inputting a list of marker points using argparse's built-in support for reading command line arguments from a file using fromfile_prefix_chars
?
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.
Are you talking about the parser.add_argument_group? That is documented here https://docs.python.org/3/library/argparse.html#argument-groups. It's purely for having the options displayed separately in the --help message (I thought the list was getting a bit long).
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.
No, its an argument for the ArgumentParser
constructor itself...
fromfile_prefix_chars
Sometimes, for example when dealing with a particularly long argument lists, it may make sense to keep the list of arguments in a file rather than typing it out at the command line. If the fromfile_prefix_chars= argument is given to the ArgumentParser constructor, then arguments that start with any of the specified characters will be treated as files, and will be replaced by the arguments they contain. For example:with open('args.txt', 'w') as fp:
... fp.write('-f\nbar')
parser = argparse.ArgumentParser(fromfile_prefix_chars='@')
parser.add_argument('-f')
parser.parse_args(['-f', 'foo', '@args.txt'])
Namespace(f='bar')
Arguments read from a file must by default be one per line (but see also convert_arg_line_to_args()) and are treated as if they were in the same place as the original file referencing argument on the command line. So in the example above, the expression ['-f', 'foo', '@args.txt'] is considered equivalent to the expression ['-f', 'foo', '-f', 'bar'].The fromfile_prefix_chars= argument defaults to None, meaning that arguments will never be treated as file references.
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.
Sorry, I'm not sure what we are talking about here then. Is your original comment about the specific --pta-file argument then? The place of this comment in the code throws me off.
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.
Yes. Is this a common file format that is in use already elsewhere? If not, can we provide equivalent functionality by simply allowing the value of any command line argument to be read from a file (including, but not limited to, the --radec
command line argument) by using this argparse feature?
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.
Ah I see. No it's just something I implemented because I needed it (I was working with simulated PTAs that stored the pulsar locations and noise levels in a txt file). Then again, a text file with comma-separated-values is very common in general, is it not? Could also change it to white-space-separated (or make it another option...).
If we want to parse the file as a command line argument instead, would we have separate files for each of Ra, Dec and the noise rms values? since each line is read as a separate argument. I don't really see how this simplifies things to be honest.
pulsar_sizes = np.full(len(pta_data), opts.pta_fix_marker_size) | ||
# scale pta marker size with noise rms: bigger markers for lower noise | ||
else: | ||
pulsar_sizes = 10 * (pta_data[:, 2] / 1.e-7)**(-0.4) |
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.
Where does this expression come from?
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.
It's completely empirical. The noise level of a typical PTA pulsar is of order 100 ns (but can easily be larger as well), so hence the scaling. The power law I just found by trying different options to see what looks good with a range of noise levels. The scaling with 10 is just a base marker size of 10. The scaling is there so that "better" (i.e. lower rms noise level pulsars) are shown with bigger markers.
If you want, I could easily add additional options to modify the scaling behaviour, for example replace the 10 with a --pta-marker-size-scale argument.
As per comments from Leo.
The following options are added that specify shape, color, edge color
and size of the marker used with the --radec option that already
existed:
--marker to specify the symbol
--marker-color for the color
--marker-ecolor for the edge color (outside line of the marker)
--marker-size for the size
Defaults are implemented so that the behaviour does not change when NOT
specifying any of these new options. I.e. by default, --radec plots a
size 10 star ('*') marker in white with a black outline.
Working on: adding options and code to plot locations of PTA pulsars using an input text file with specified locations and pulsar noise levels.