Configurability for material plots #392
Conversation
Victor-Schwan
force-pushed
the
PR_utils_config
branch
2 times, most recently
from
0c8bf66
to
f2cd8da
Compare
Victor-Schwan
force-pushed
the
PR_utils_config
branch
2 times, most recently
from
b0f3ac7
to
62be7fa
Compare
| parser.add_argument('--angleBinning', "-b", dest='angleBinning', default=0.05, type=float, help="eta/theta/cosTheta bin width") | ||
| parser.add_argument('--nPhiBins', dest='nPhiBins', default=100, type=int, help="number of bins in phi") | ||
| parser.add_argument('--x0max', "-x", dest='x0max', default=0.0, type=float, help="Max of x0") | ||
| parser = argparse.ArgumentParser(description="Material Plotter") |
There was a problem hiding this comment.
Can we please not mix style changes and feature changes?
There was a problem hiding this comment.
That was not my intention. I have activated auto-formatting (black + isort) in code and as soon as I open a script it is also formatted. In general, I think autoformatting makes a lot of sense in a project like this...
There was a problem hiding this comment.
The issue in this case is not the re-formatting itself, but rather that the formatting changes and feature changes are mixed in one commit. In case there is an auto-formatter, the way to go would be: auto-format only first in a separate commit and then make a second commit with the feature changes.
That makes it easier to review the feature changes only, because we have a diff with only that. Additionally, git can be configured to ignore format only commits in git blame, which again makes it easier to figure out what actually changed in the future.
There was a problem hiding this comment.
That was not my intention. I have activated auto-formatting (
black+isort) in code and as soon as I open a script it is also formatted. In general, I think autoformatting makes a lot of sense in a project like this...
To put it bluntly and exaggerating :): No, it makes no sense like this, everyone using a different auto linter leads to way too much noise. When in Rome do as the Romans. When there is no linter config, don't lint by default.
So in this case the issue is the re-formatting :)
Yes auto-formatting is nice, but sadly there is a cost for "legacy" code.
There was a problem hiding this comment.
I see. I think I managed to add a commit that isolates the style changes :)
|
Now can we have those changes on top of the original style 🤪 |
|
I am afraid that is not something I can do in 5 minutes ;) Should we format all python sources and make a PR that we can then ignore in blames? |
But something that reduces the amount of changes. Because #396 is going to be painful enough to merge. |
| from os import fspath | ||
| from os.path import expandvars |
There was a problem hiding this comment.
Do we really have to import these, and not just do import os and then os.fspath and os.path.expandvars below? Would keep the diff cleaner and make it much easier to search for usages.
There was a problem hiding this comment.
I think it is more pythonic, shortens the following code (preventing newlines), and prevents namespace clutter. One can still search for fspath and expandvars. But it is not a matter of life-and-death for me
| parser.add_argument('--x0max', "-x", dest='x0max', default=0.0, type=float, help="Max of x0") | ||
| parser = argparse.ArgumentParser(description="Material Plotter") | ||
| parser.add_argument( | ||
| "--inputFile", "-f", type=str, help="relative path to the input file" |
There was a problem hiding this comment.
No pressing need to change the argument from --fname. Changing user interfaces should be avoided unless absolutely necessary., add --inputFile as an alternate if you really want to?
There was a problem hiding this comment.
I thought of it for two reasons:
- That way, the argument name is more consistent with ILDConfig. I prefer if scripts name similar arguments similarly
- The name does not conform to camelCase which most arguments do
If it is used frequently, keeping --fname is certainly an option
There was a problem hiding this comment.
Let me say that I can see where you are coming from, but really the big picture matters here, and you have to consider other people as well. Your preferences are not supreme.
So, no changes to user interfaces unless absolutely necessary. There might be existing documentation somewhere, people have scripts that call this script with this argument. Or the original author just preferred fname and things are as they are, that ship has sailed.
Or as Linus puts it "WE DO NOT BREAK USERSPACE!"
(the expletive in that link is not aimed at you), but I have / had to deal with too many callous interface changes in the past.
So, if you want to add inputName that is OK, you have a valid argument for that. But you do not have any valid argument to break existing usage.
There was a problem hiding this comment.
Fair point. Thanks for the explanation. I will make sure that the user interface is not broken :)
There was a problem hiding this comment.
Thanks for your understanding!
Victor-Schwan
force-pushed
the
PR_utils_config
branch
from
62be7fa
to
932d5b4
Compare
…angleDef option added to material_plots_2D.py
Victor-Schwan
force-pushed
the
PR_utils_config
branch
from
932d5b4
to
80d9cc4
Compare
Co-authored-by: Thomas Madlener <[email protected]>
BEGINRELEASENOTES
material_plots_2D.py:angleDefis a choice parameter +thetaRadchoice addedoutputDiroption addedmaterial_scan_2d.py:compactFile,outputFileBase,angleDef)outputDiroption addedENDRELEASENOTES