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

Modify vp-comparefits interface #1312

Merged
merged 2 commits into from
Jul 7, 2021
Merged

Conversation

scarlehoff
Copy link
Member

vp-comparefits requires a number of arguments to be added when ran from command-line, however the --help doesn't really help on knowing which they are. I've grouped them so that one can quickly know which are mandatory:

mandatory:
  Mandatory command line arguments

  --title TITLE         The title that will be indexed with the report.
  --author AUTHOR       The author of the report.
  --keywords KEYWORDS [KEYWORDS ...]
                        keywords to index the report with.

Then there're the flags th_covmat_if_present and no-thcovmat_if_present which are both the same one. I guess there might be a good reason why having one of the two was made mandatory but I think it makes more sense to be false by default and use it if needed? I've put as reviewer people who modified this file just in case I broke something important...

Note: I've separated the thcovmat and the groupping commits so I can revert the thcovmat if there's a good reason for making it mandatory.

@Zaharid
Copy link
Contributor

Zaharid commented Jun 29, 2021

Looks sensible.

@wilsonmr
Copy link
Contributor

wilsonmr commented Jun 29, 2021

Then there're the flags th_covmat_if_present and no-thcovmat_if_present which are both the same one. I guess there might be a good reason why having one of the two was made mandatory but I think it makes more sense to be false by default and use it if needed?

The flag was set to true by default because then it usually does the sensible. thing - uses theory covmat if one was used in the fit and vice versa. The name was supposed to reflect that if there is no theory covmat it still does something sensible* but perhaps it's confusing at the moment considering adding theory covmat is basically unsupported. No strong feeling either way

*absolutely nothing

@scarlehoff
Copy link
Member Author

It might be better then to set it to true by default (i.e., the inverse of what I did, deprecate the thvcovmat instead of the no-thcovmat)? (I'm also fine with both)

@scarrazza scarrazza merged commit fbbca0b into master Jul 7, 2021
@scarrazza scarrazza deleted the modify_commparefits_interface branch July 7, 2021 14:10
@Zaharid Zaharid added the enhancement New feature or request label Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants