-
Notifications
You must be signed in to change notification settings - Fork 26
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
Reformat model information and add link to full channel list #78
Conversation
bin/gwdetchar-lasso-correlation
Outdated
except(IOError, IndexError): | ||
numpy.savetxt(channelsFile, channels, fmt="%s") | ||
except RuntimeError as e: | ||
if 'latex' in str(e).lower(): |
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.
@macedo22, why would latex get involved here?
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.
@duncanmmacleod Not sure. I am not too familiar with Latex and because this exception handling was used for saving all of the plots, I assumed it would go here too. I figured the handling was to account for characters in strings that Latex would get mad at, but it sounds like what you're saying is a Latex exception wouldn't be raised from saving channel names to a text file, as it could for images. I will remove the if/else conditional here and just have
except RuntimeError:
raise
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.
there should be no need for any exception handling here at all, so I recommend you remove all of it, unless you understand a configuration where the file writing should be allowed to fail.
Hi @duncanmmacleod, I removed the error handling you requested as well as a few other things.
|
bin/gwdetchar-lasso-correlation
Outdated
@@ -99,20 +100,15 @@ parser.add_argument('-J', '--nproc-plot', type=int, default=None, | |||
parser.add_argument('-o', '--output-dir', default=os.curdir, | |||
help='output directory for plots') | |||
parser.add_argument('-f', '--channel-file', type=os.path.abspath, | |||
help='path for channel file') | |||
help='path for channel file', required=True) |
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.
@macedo22, I'm not a fan of required optional arguments, since that makes no sense? Can you have a think about changing this to a positional argument if it really is required
?
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.
@duncanmmacleod Yes, I can! I can just remove it to keep options open, as you say
bin/gwdetchar-lasso-correlation
Outdated
@@ -128,7 +124,7 @@ psig.add_argument('-x', '--filter-padding', type=float, default=3., | |||
lsig = parser.add_argument_group('LASSO options') | |||
lsig.add_argument('-a', '--alpha', default=None, type=float, | |||
help='alpha parameter for LASSO fit') | |||
lsig.add_argument('-C', '--cluster', default=True, type=bool, | |||
lsig.add_argument('-C', '--cluster', default='True', type=str, |
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 don't understand this change, can you explain?
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.
@duncanmmacleod As I discovered from troubleshooting and reading more online, for some reason, boolean arguments will always be True
whether you pass in True
or False
. There are a few different ways to work around this issue, but I found this the simplest and most readable.
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 believe it has something to do with the nature of python or parsing. It will treat any string passed as a boolean as True
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.
in this case you want to use action='store_true'
and just give the argument with a value, e.g.
lsig.add_argument('-C', '--cluster', action='store_true',
help='generate clustered channel plots')
the user should then pass just --cluster
to enable the clustering.
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.
@duncanmmacleod I'm happy to change this, but our overall usage preference has been to always do clustering by default, and only turn it off if in special cases (e.g. when troubleshooting the code overall to fix bugs). If we would like to keep it as doing clustering by default without the user passing anything, is there another way you would like me to rewrite it? If you still feel this new way you proposed is best, I will make the switch.
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.
@macedo22, you can use the opposite logic and provide something like:
lsig.add_argument('-C', '--no-cluster', action='store_true', default=False,
help='do not generate clustered channel plots')
I believe that one of those options using action='store_true'
is better than having the user manually pass true
or false
on the command-line.
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.
@duncanmmacleod Sounds good to me. I swapped this in and tested it, so it should be good to go with this most recent commit 6dd7498
.
bin/gwdetchar-lasso-correlation
Outdated
auxdata = gooddata | ||
flattab=Table(data = (numpy.asarray(flatdata.keys()),), names = ('Channels',)) |
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.
@macedo22, please use pep8 white-spacing standards throughout.
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.
@duncanmmacleod will do!
@duncanmmacleod I went ahead and put in your edits. In addition, I have swapped out |
bin/gwdetchar-lasso-correlation
Outdated
write_param('Model', 'LASSO') | ||
write_param('Non-zero coefficients', '%d' % numpy.count_nonzero(model.coef_)) | ||
write_param('Alpha', '%g' % usedalpha) | ||
write_param('Zero coefficients','%d (%s)' % (len(zeroedtab), "<a href= %s target='_blank'>zeroed channel list</a>" % (zerofile))) | ||
page.p('<br /><br />%s' % df.to_html(index=True)) | ||
page.div.close()#model-info |
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.
as per pep8, please leave two spaces before inline comments:
page.div.close() # model-info
Hi @duncanmmacleod, I did an overhaul of the code in an attempt to make it more robust. The changes include: |
An updated example page output: https://ldas-jobs.ligo-la.caltech.edu/~alexandra.macedo/detchar/LASSO/test/O2/L1/20170301/1172363755-1172375019/ |
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.
LGTM
bin/gwdetchar-lasso-correlation
Outdated
return plot7, plot7_legend | ||
|
||
|
||
if args.cluster is True: | ||
if args.no_cluster is False: |
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.
@macedo22, by convention its normally better to use if not args.no_cluster
for this (and similarly just if args.cluster
for truth testing).
@macedo22, this looks fine to me, the CI failure is from somewhere else and is related to backwards-incompatible gwpy changes that haven't been addressed in gwdetchar yet, I'll get to it soon. |
@duncanmmacleod Great! It seemed like the issue was in |
@duncanmmacleod Done and done! |
No description provided.