Skip to content

Start updating code for ctapipe 0.25 #1361

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

Merged
merged 74 commits into from
Apr 30, 2025
Merged

Start updating code for ctapipe 0.25 #1361

merged 74 commits into from
Apr 30, 2025

Conversation

moralejo
Copy link
Collaborator

We should move on with this (even update to more recent ctapipe & ctapipe_io_lst), but to be honest I do not know how much needs to be updated in lstchain...

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

@moralejo You should update the ctapipe version tested in CI in .github/workflows

@moralejo
Copy link
Collaborator Author

@moralejo You should update the ctapipe version tested in CI in .github/workflows

thx, will do

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

It seems there is also an incompatibility with gammapy

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

I think this is between scipy and gammapy and should be fixed by updating gammapy to 1.2

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

Add MissingReferenceMetadata to the ignored warnings in pyproject.toml

@moralejo
Copy link
Collaborator Author

Add MissingReferenceMetadata to the ignored warnings in pyproject.toml

What will that do?

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

E File "/home/runner/work/cta-lstchain/cta-lstchain/lstchain/scripts/lstchain_data_r0_to_dl1.py", line 230, in main
E r0_to_dl1.r0_to_dl1(
E File "/home/runner/work/cta-lstchain/cta-lstchain/lstchain/reco/r0_to_dl1.py", line 718, in r0_to_dl1
E event.r1.tel[telescope_id].waveform *= ~bad_waveform
E ValueError: operands could not be broadcast together with shapes (1,1855,36) (1855,1855) (1,1855,36)

ctapipe has changed the shape of waveform to always be 3d, also for gain selected data.

@moralejo
Copy link
Collaborator Author

Add MissingReferenceMetadata to the ignored warnings in pyproject.toml

What will that do?

I still get the warnings, not sure what I did wrong...

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

I still get the warnings, not sure what I did wrong...

I don't see them anymore in the current run. They are also gone if you update to a newer version of ctapipe. We were a bit too heavy on this warning, raising it also for files we know can not have this metadata.

@maxnoe
Copy link
Member

maxnoe commented Apr 24, 2025

In current ctapipe, the extractors should just return a 2d image if you give it all gains and no selected_gain_channel, so the worarounds here should no longer be necessary

should be unnecessary with current ctapipe
@moralejo moralejo changed the title Start updating code for ctapipe 0.22 Start updating code for ctapipe 0.25 Apr 24, 2025
@moralejo
Copy link
Collaborator Author

Now tests will fail again in the (DL1ab) NSB tuning, I put back the original reference value. Once we understand why the integration correction is not applied (at least for the interleaved pedestals) and solve it, the test should pass.

Solved now

@moralejo
Copy link
Collaborator Author

I think the remaining non-passing tests will be solved with this issue pointed out by @maxnoe:
cta-observatory/ctapipe#2744
So we need a bugfix release of ctapipe to proceed.

@maxnoe
Copy link
Member

maxnoe commented Apr 28, 2025

I plan to do it today

@moralejo
Copy link
Collaborator Author

moralejo commented Apr 29, 2025

The only pending failing test was working for me "locally" (i.e. at the cluster) but I cannot test now - bridge computer still down.
Anyway, it looks like the output of the calibration is not what it used to be, and hence the error in the plotting part. @FrancaCassol can you have a look?

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

Anyway, it looks like the output of the calibration is not what it used to be, and hence the error in the plotting part.

Did you check that there is an actual change in the output? Maybe it's just the new matplotlib version complaining about it where the old one didn't.

What version of mpl do you have locally? Is it the up-to-date 3.10 as it is in the CI?

@moralejo
Copy link
Collaborator Author

Anyway, it looks like the output of the calibration is not what it used to be, and hence the error in the plotting part.

Did you check that there is an actual change in the output? Maybe it's just the new matplotlib version complaining about it where the old one didn't.

No I couldn't check yet, it was just speculation. On the other hand, it did not fail in my local test, which is weird.
In any case, auto-axis setting failing when nans or infs are present used to be there, right? (I've found it often)

What version of mpl do you have locally? Is it the up-to-date 3.10 as it is in the CI?

Can't connect to the IT yet...

@FrancaCassol
Copy link
Collaborator

Hi @maxnoe

Did you check that there is an actual change in the output? Maybe it's just the new matplotlib version complaining about it where the old one didn't.

What version of mpl do you have locally? Is it the up-to-date 3.10 as it is in the CI?

Locally the test works for me and I have mpl 3.10.1 ...

@moralejo
Copy link
Collaborator Author

Hi @maxnoe

Did you check that there is an actual change in the output? Maybe it's just the new matplotlib version complaining about it where the old one didn't.
What version of mpl do you have locally? Is it the up-to-date 3.10 as it is in the CI?

Locally the test works for me and I have mpl 3.10.1 ...

Same here. I cannot understand, the input file is the same as it used to be, so it is not about a change in the contents. And the point of failure seems to be just creating a camera display, before values are set in.

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

Does the test work locally when you set MPLBACKEND=Agg? Maybe it's a difference in mpl backends and the CI does not have a proper display environment.

@moralejo
Copy link
Collaborator Author

Does the test work locally when you set MPLBACKEND=Agg? Maybe it's a difference in mpl backends and the CI does not have a proper display environment.

It still works when I set MPLBACKEND=Agg (as env variable)

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

I assume you ran this test in isolation?

I think the issue is that if all tests are run, there is a conflict in figure numbers and the wrong figure is used to plot.

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

I removed the explicit figure numbers, we had a similar issue in lstcam_calib

Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 73.43750% with 17 lines in your changes missing coverage. Please review.

Project coverage is 72.65%. Comparing base (6db577d) to head (7659454).
Report is 75 commits behind head on main.

Files with missing lines Patch % Lines
lstchain/scripts/lstchain_longterm_dl1_check.py 6.66% 14 Missing ⚠️
lstchain/visualization/bokeh.py 33.33% 2 Missing ⚠️
lstchain/datachecks/containers.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1361      +/-   ##
==========================================
- Coverage   72.67%   72.65%   -0.02%     
==========================================
  Files         137      137              
  Lines       14552    14559       +7     
==========================================
+ Hits        10575    10578       +3     
- Misses       3977     3981       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@FrancaCassol
Copy link
Collaborator

FrancaCassol commented Apr 29, 2025

I assume you ran this test in isolation?

I personally run all the tests (in cp02), all worked except test_write_dataframe() whit the error :

OSError: Can't synchronously read data (can't open directory (/usr/local/hdf5/lib/plugin). Please verify its existence)

@moralejo
Copy link
Collaborator Author

moralejo commented Apr 29, 2025

I assume you ran this test in isolation?

I think the issue is that if all tests are run, there is a conflict in figure numbers and the wrong figure is used to plot.

For me, locally, it worked both running in isolation and running them all.
When running them all at once, I got that one test "xfailed" - couldn't find it - was that it?

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

Ok, but that was it, tests are passing now

@maxnoe
Copy link
Member

maxnoe commented Apr 29, 2025

A remaining difference could be that the CI runs tests in parallel (-n 2)

@moralejo moralejo marked this pull request as ready for review April 29, 2025 15:33
"transform_waveform": true,
"waveform_dtype": "uint16",
"waveform_offset": 400,
"waveform_scale": 80
"waveform_scale": 60
Copy link
Member

Choose a reason for hiding this comment

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

@FrancaCassol Fine with changing the scale here?

Should we change the offset to 300 so that it is equivlent to the 60/5 we use in the EVB?

@moralejo moralejo merged commit 46392e4 into main Apr 30, 2025
7 of 8 checks passed
@moralejo moralejo deleted the update_deps branch April 30, 2025 11:55
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