-
Notifications
You must be signed in to change notification settings - Fork 19
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
Work on cli tools #215
Work on cli tools #215
Conversation
@NikoOinonen, @ProkopHapala, I quickly tested things, but might be I missed something. Please have a look and test as well. |
# To fix. | ||
# if common.params["tip"] == ".py": | ||
# # import tip | ||
# exec(compile(open("tip.py", "rb").read(), "tip.py", "exec")) | ||
# print(tipMultipole) | ||
# common.params["tip"] = tipMultipole | ||
# print("params['tip'] ", common.params["tip"]) |
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.
What is this actually? Looks kinda hacky. Does removing this break any functionality? @ProkopHapala This is your code, I think.
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.
this allows the user to specify tip Multipole as dictionary in tip.py
file
it is hacky, but why not
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 think it's kind of confusing, because the tipMultipole
just comes into existence out of nowhere and it's difficult to know what it's supposed to be if you don't already have an example of a tip.py
file at hand.
It also triggers automated error checkers with messages like "tipMultipole" is not defined
, which I'm guessing is the reason why @yakutovicha commented it out.
I would maybe just define an interface through the CLI arguments for this. Something like
--tip 'dz2' -0.1 'pz' 0.05 's' -0.05
should be quite easy to parse into a dictionary.
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.
cannot we just pass it as string like
"{'dz2':-0.1,'pz':0.05,'s':-0.05}"
and than exec it?
something like this:
if isinstance( common.params["tip"], str):
exec( 'tipMultipole='+common.params["tip"] )
common.params["tip"] = tipMultipole
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 made a new issue for it: #221.
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.
Just a few minor things that I noticed.
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!
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 actually see what are the changes.
Just the formatng?
But I don't see obvious reason why not to approve it
# To fix. | ||
# if common.params["tip"] == ".py": | ||
# # import tip | ||
# exec(compile(open("tip.py", "rb").read(), "tip.py", "exec")) | ||
# print(tipMultipole) | ||
# common.params["tip"] = tipMultipole | ||
# print("params['tip'] ", common.params["tip"]) |
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.
this allows the user to specify tip Multipole as dictionary in tip.py
file
it is hacky, but why not
In this PR I revised our CLI tools to further implement smoke testing.