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

Do not redo link to pro directory if already correct #896

Merged
merged 6 commits into from
Feb 1, 2022

Conversation

FrancaCassol
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #896 (eca3adf) into master (b22332d) will increase coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   83.83%   84.29%   +0.46%     
==========================================
  Files          77       84       +7     
  Lines        5968     6704     +736     
==========================================
+ Hits         5003     5651     +648     
- Misses        965     1053      +88     
Impacted Files Coverage Δ
lstchain/__init__.py 77.27% <0.00%> (-5.09%) ⬇️
lstchain/io/config.py 100.00% <0.00%> (ø)
lstchain/io/__init__.py 100.00% <0.00%> (ø)
lstchain/io/tests/test_config.py 100.00% <0.00%> (ø)
lstchain/high_level/tests/test_hdus.py 100.00% <0.00%> (ø)
lstchain/high_level/hdu_table.py 92.18% <0.00%> (ø)
lstchain/data/normalised_pulse_template.py 100.00% <0.00%> (ø)
...chain/data/tests/test_normalised_pulse_template.py 100.00% <0.00%> (ø)
lstchain/data/__init__.py 100.00% <0.00%> (ø)
lstchain/high_level/__init__.py 100.00% <0.00%> (ø)
... and 6 more

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 b22332d...eca3adf. Read the comment docs.

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.

Could you maybe put that code into a function

def create_pro_link(output_dir):

It is duplicated 4 times and you now had to make a change at 4 locations in exactly the same way.

@rlopezcoto rlopezcoto changed the title Do not redo link to pro directory of already correct Do not redo link to pro directory if already correct Feb 1, 2022
@FrancaCassol
Copy link
Collaborator Author

Hi @maxnoe,

where do you want to put this function?

@maxnoe
Copy link
Member

maxnoe commented Feb 1, 2022

@FrancaCassol maybe create a new module lstchain/onsite.py?

I wanted to also refactor the rest of the duplicated code in the onsite scripts, e.g. by introducing functions like:

def find_drs4_baseline_run(run_id, base):
    ...

@FrancaCassol
Copy link
Collaborator Author

I see, do we want to do it now or we better keep it for later?

@maxnoe
Copy link
Member

maxnoe commented Feb 1, 2022

We can also do it in one go later...

maxnoe
maxnoe previously approved these changes Feb 1, 2022
@rlopezcoto
Copy link
Contributor

wait, I was modifying it after talking to @FrancaCassol

@FrancaCassol
Copy link
Collaborator Author

If there are other changes I think it is better to do all later.
I would merge it as it is now

@rlopezcoto
Copy link
Contributor

@FrancaCassol maybe create a new module lstchain/onsite.py?

I created instead a path.py module inside scripts/onsite, does that sound good?

@FrancaCassol
Copy link
Collaborator Author

I liked better the onsite.py idea that is more general.

@rlopezcoto rlopezcoto merged commit 6f3fbc2 into master Feb 1, 2022
@rlopezcoto rlopezcoto deleted the correct_pro_link_production branch February 1, 2022 12:16
@rlopezcoto rlopezcoto mentioned this pull request Feb 2, 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