-
Notifications
You must be signed in to change notification settings - Fork 2
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 corrections eff #114
base: master
Are you sure you want to change the base?
Add corrections eff #114
Conversation
cax/main.py
Outdated
@@ -187,6 +187,9 @@ def massive(): | |||
help="Select the cluster partition") | |||
parser.add_argument('--reservation', type=str, | |||
help="Select the reservation") | |||
parser.add_argument('--local', action='store_true', | |||
help="Select only raw datasets which are stored locally at a host") |
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 thought "local" meant running not in batch queue (not submitting jobs)
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 changed it to no-batch-mode to avoid confusion.
cax/main.py
Outdated
|
||
for doc in docs: | ||
|
||
#Run certains actions only on local file. |
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 guess you mean: processes only on runs that satisfy the condition below (non-existence of 'correction_versions')
cax/main.py
Outdated
#ask if data field processor/correction_versions exists | ||
#if not start to apply corrections | ||
if 'processor' in doc: | ||
if 'correction_versions' not in doc['processor']: |
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.
can you add another flag that toggles this requirement? (e.g. in case we want to run in local mode regardless)
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 use the task_list to avoid to many option which can then used in a miss leading way.
@XeBoris please check the codacy report too |
@lucrlom can you please review and test this on |
cax/main.py
Outdated
|
||
#Define basic command: | ||
basic_command = """#!/bin/bash | ||
cax --once {config} --name {name} |
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.
indentation problem maybe
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.
should be ok for a string definition, similar to this
cax/main.py
Outdated
task_list = config.get_config(config.get_hostname())['task_list'] | ||
|
||
#This is a list of tasks which refer to apply corrections to a run: | ||
task_list_correction = ['AddElectronLifetime', 'AddGains', 'AddDriftVelocity', 'SetS2xyMap', 'SetLightCollectionEfficiency', 'SetFieldDistortion', 'SetNeuralNetwork'] |
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'd like put a warning here. If the user doesn't select json file
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.
Maybe I'd like to add only a warning to remember that by default we are adding a correction at line 314 in main.py.
cax/main.py
Outdated
|
||
#-------------------- | ||
#Start with single activities: Apply corrections only to data sets which do not have any correction yet: | ||
if bool(set(task_list) & set(task_list_correction)) or task_list_correction == task_list: |
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'm not so familiar with this bitwise operation on Python sets, but isn't the second condition after the or
included in the first condition (i.e. don't need the or
)?
Also, what if I want to run in no-batch mode without running corrections?
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 know exactly what does set, maybe it creates a "set" of elements, but yes, the second command should not to be necessary.
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.
Yes it is included and you would not mind if you run the full set of corrections such as they are defined in line (
Line 314 in 144fd4e
task_list_correction = ['AddElectronLifetime', 'AddGains', 'AddDriftVelocity', 'SetS2xyMap', 'SetLightCollectionEfficiency', 'SetFieldDistortion', 'SetNeuralNetwork'] |
In case we come up with more corrections we would need to adjust line 314. But I think this is ok.
@lucrlom did you ever test this on
Anyway, it looks like the So I guess we should just merge as long as you confirm default behavior (i.e. without |
This change in massive-cax works together with a new "--local" to change the batch queue purpose of massive-cax into simple submission of simple cax jobs (selected by run number or run name).
This change is regarding adding corrections to the runDB only if its necessary then and allows to cycle much faster over the whole runDB.
Fixes #108