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

Store true #150

Merged
merged 13 commits into from
Feb 25, 2022
Merged

Store true #150

merged 13 commits into from
Feb 25, 2022

Conversation

vuillaut
Copy link
Member

@vuillaut vuillaut commented Feb 2, 2022

No description provided.

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #150 (6d02ca1) into master (2cb8bc6) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
- Coverage   14.07%   14.04%   -0.03%     
==========================================
  Files          33       33              
  Lines        2068     2072       +4     
==========================================
  Hits          291      291              
- Misses       1777     1781       +4     
Impacted Files Coverage Δ
lstmcpipe/hiperta/hiperta_r0_to_dl1lstchain.py 0.00% <ø> (ø)
lstmcpipe/scripts/script_batch_filelist_rta.py 0.00% <0.00%> (ø)
lstmcpipe/stages/mc_process_dl1.py 0.00% <0.00%> (ø)

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 2cb8bc6...6d02ca1. Read the comment docs.

@garciagenrique
Copy link
Collaborator

Sorry the prod was launched, won't be merging, at least for the moment

@LukasNickel
Copy link
Member

Why can we not merge when a production is running?

@garciagenrique
Copy link
Collaborator

Why can we not merge when a production is running?

Wanted to tag before the exact version that created the HiPeRTa MC prod.

@garciagenrique
Copy link
Collaborator

And also because the PR is missing the corresponding changes to the strings that call these entry poins.

Copy link
Collaborator

@garciagenrique garciagenrique left a comment

Choose a reason for hiding this comment

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

Change also the way these entry points are called in the rest of the code, not only how the arguments are passed.

@vuillaut
Copy link
Member Author

vuillaut commented Feb 7, 2022

Change also the way these entry points are called in the rest of the code, not only how the arguments are passed.

Done.
lstmcpipe/scripts/script_batch_filelist_rta.py is not called anywhere, right?

@LukasNickel
Copy link
Member

Change also the way these entry points are called in the rest of the code, not only how the arguments are passed.

Done. lstmcpipe/scripts/script_batch_filelist_rta.py is not called anywhere, right?

It seems to be called here:

f"{source_environment} lstmcpipe_rta_core_r0_dl1 -c {config_file} "

@@ -129,6 +129,7 @@ def r0_to_dl1(
workflow_kind="lstchain",
keep_rta_file=False,
n_jobs_parallel=20,
debug_mode=False,
Copy link
Member

Choose a reason for hiding this comment

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

I see no way to set this, how do you want to use it?

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be the debug flag we already have in lstmcpipe_start.py?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hhmm right... 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, there are functionalities that were implemented long time ago, but now I'm not sure they make much sense.... (before you could configured HiPeRTA from the config file...). Same for keep_files indeed.

Copy link
Collaborator

@garciagenrique garciagenrique Feb 11, 2022

Choose a reason for hiding this comment

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

I would propose.
Nobody uses hiperta functionality but me.
Let's not add extra args to the config file that will never be used.
Either I implement some optional args when hiperta workflow is set (and open an issue otherwise I will forgot), either (what would be most probable) I change the default value of the vars on the script by hand and I re-install if ever I need to used these args 🙈 🙈 🙈 🙈 - not that I do this always 😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, now I recall. Whenever I needed these functionalities, I was directly changing things on the old and deprecated batch_files_rta.sh.

@garciagenrique
Copy link
Collaborator

Would give a though how to do things correctly, leaving the PR open for the moment.

@vuillaut
Copy link
Member Author

@garciagenrique ?

@garciagenrique garciagenrique merged commit e181fa5 into master Feb 25, 2022
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.

3 participants