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

Add no-flags, add short aliases, make aliases more consistent #1748

Merged
merged 6 commits into from
Jun 4, 2021

Conversation

nbiederbeck
Copy link
Contributor

Fixes #1738

ctapipe/tools/display_dl1.py Outdated Show resolved Hide resolved
ctapipe/tools/dl1_merge.py Outdated Show resolved Hide resolved
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

The for loop looks strange in the class body,

it should be possible to just define flags like this, right?

flags = {
    **flag(...),
    **flag(...),
   ...
}

Also, there are some boolean flags remaining, that are not setup using flag, can you transform those as well?

@nbiederbeck
Copy link
Contributor Author

I thought the loop is the most elegant solution because I forgot about **

@nbiederbeck
Copy link
Contributor Author

do we want a --no-progress flag?

@maxnoe
Copy link
Member

maxnoe commented Jun 4, 2021

Yes, I think that makes sense. In case we switch the default for example. Having all boolean flags consistent and automatically negated is a big plus I'd say.

@nbiederbeck
Copy link
Contributor Author

That leads me to another question: the should the (-f, --overwrite) flags (and e.g. (-q, --quiet)) also become negated? But only the long forms, I suppose? (e.g. no --no-f)

@maxnoe
Copy link
Member

maxnoe commented Jun 4, 2021

shortform negation doesn't make sense, correct. How does flag do shortforms?

@nbiederbeck
Copy link
Contributor Author

afaik it doesn't. It can't handle short/long tuples, only strings. So I think we need to add a long yes/no flag and the short form we have to build ourselves

@maxnoe
Copy link
Member

maxnoe commented Jun 4, 2021

It doesn't. And it also does not make sure that only one of the flags is given.

@nbiederbeck
Copy link
Contributor Author

What do you mean exactly? When I give --flag --no-flag it takes the last one. When I give -l --long it doesn't really matter

@maxnoe
Copy link
Member

maxnoe commented Jun 4, 2021

I somehow expected that traitlets would raise an error for --flag --no-flag

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #1748 (b17b785) into master (9fb2901) will decrease coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1748      +/-   ##
==========================================
- Coverage   91.62%   91.28%   -0.34%     
==========================================
  Files         185      185              
  Lines       14382    14380       -2     
==========================================
- Hits        13177    13127      -50     
- Misses       1205     1253      +48     
Impacted Files Coverage Δ
ctapipe/tools/tests/test_tools.py 97.14% <ø> (-0.03%) ⬇️
ctapipe/tools/display_dl1.py 61.53% <100.00%> (-28.94%) ⬇️
ctapipe/tools/display_integrator.py 96.32% <100.00%> (-0.06%) ⬇️
ctapipe/tools/dl1_merge.py 82.10% <100.00%> (+0.09%) ⬆️
ctapipe/tools/dump_triggers.py 86.66% <100.00%> (ø)
ctapipe/tools/muon_reconstruction.py 90.98% <100.00%> (+0.07%) ⬆️
ctapipe/tools/stage1.py 93.75% <100.00%> (ø)
ctapipe/tools/camdemo.py 55.68% <0.00%> (-20.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb2901...b17b785. Read the comment docs.

@maxnoe maxnoe merged commit 15cd535 into cta-observatory:master Jun 4, 2021
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.

Stage1 tool's --write-parameters flag is superflous
3 participants