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

Remove some obsolete TODOs #2573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Jul 3, 2024

I left a few in where they seemed to be still unresolved, or where it wasn't clear (the latter was particularly in table_interpolator.py and unstructured_interpolator.py).

@@ -244,12 +244,6 @@ def _calibrate_dl1(self, event, tel_id):
waveforms -= np.atleast_2d(dl1_calib.pedestal_offset)[..., np.newaxis]

if n_samples == 1:
# To handle ASTRI and dst
Copy link
Member

Choose a reason for hiding this comment

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

I think this one still applies (see #1777 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still want to support DSTs? if so, should that be written here as a TODO? I would expect it to be an issue, but nothing in the code.

@maxnoe
Copy link
Member

maxnoe commented Jul 8, 2024

@kosack the CI run fails because the job from your fork doesn't have access to SONAR_TOKEN.

Which is really not good and we should have noticed earlier... how to we proceed here? Convert the SONAR_TOKEN from a secret to a simple env variable? Doesn't seem like a good idea.

@kosack
Copy link
Contributor Author

kosack commented Jul 9, 2024

Which is really not good and we should have noticed earlier... how to we proceed here? Convert the SONAR_TOKEN from a secret to a simple env variable? Doesn't seem like a good idea.

I could also just push it to upstream and re-open the PR. But it is a general problem if people develop in their own forks. Not sure how that is usually dealt with? In larger open-source projects, I can't imagine everyone sets up sonar tokens for their own forks

@maxnoe
Copy link
Member

maxnoe commented Jul 9, 2024

See the issue I created: #2577 and an attempt at a fix I found in the sonarqube forums: #2578

@maxnoe
Copy link
Member

maxnoe commented Jul 9, 2024

You could leave this open here so we can test after merging #2578

@maxnoe
Copy link
Member

maxnoe commented Jul 9, 2024

sonarqube reporting should work again now, let's see

Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage No coverage information (94.20% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.60% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@maxnoe
Copy link
Member

maxnoe commented Jul 10, 2024

FINALLY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants