Skip to content

Make muon analysis part of the processing tool#2168

Merged
kosack merged 36 commits into
masterfrom
muon_data_writer
Jan 26, 2023
Merged

Make muon analysis part of the processing tool#2168
kosack merged 36 commits into
masterfrom
muon_data_writer

Conversation

@StFroese
Copy link
Copy Markdown
Member

@StFroese StFroese commented Dec 15, 2022 •
edited
Loading

No description provided.

maxnoe
maxnoe reviewed Dec 15, 2022
View reviewed changes
Comment thread ctapipe/io/datawriter.py Outdated
@RuneDominik
Copy link
Copy Markdown
Member

RuneDominik commented Dec 15, 2022

Use this instead of #1618

@StFroese StFroese marked this pull request as ready for review December 15, 2022 16:37
@RuneDominik RuneDominik mentioned this pull request Dec 15, 2022
Closed
@StFroese StFroese changed the title Switch to DataWriter for muon tool Make muon analysis part of the processing tool Dec 15, 2022
nbiederbeck
nbiederbeck reviewed Dec 16, 2022
View reviewed changes
Comment thread ctapipe/containers.py Outdated
Comment thread ctapipe/image/muon/tests/test_processor.py Outdated
Comment thread ctapipe/image/muon/tests/test_processor.py
Comment thread ctapipe/tools/muon_reconstruction.py Outdated
Comment thread ctapipe/tools/muon_reconstruction.py Outdated
Comment thread ctapipe/tools/tests/test_tools.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py
for i in range(3):
ring = self.ring_fitter(x, y, image, mask)
dist = np.sqrt((x - ring.center_x) ** 2 + (y - ring.center_y) ** 2)
mask = np.abs(dist - ring.radius) / ring.radius < 0.4
Copy link
Copy Markdown
Contributor

@nbiederbeck nbiederbeck Dec 16, 2022

Choose a reason for hiding this comment

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

magic number? 0.4

configurable?

Copy link
Copy Markdown
Member Author

@StFroese StFroese Dec 16, 2022

Choose a reason for hiding this comment

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

This line seems to be the cause of #740

Copy link
Copy Markdown
Member

@maxnoe maxnoe Dec 21, 2022

Choose a reason for hiding this comment

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

@moralejo

Copy link
Copy Markdown
Member

@maxnoe maxnoe Jan 20, 2023

Choose a reason for hiding this comment

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

Let's keep the behavior the same for now and just have the Tool refactoring changes. Another PR should be made for actual behavorial changes.

@RuneDominik
Copy link
Copy Markdown
Member

RuneDominik commented Dec 16, 2022 •
edited
Loading

As of now, remaining tasks for this PR are (alongside the comments above ofc):

  • Restructure existing tests
  • Remove old muon analysis tool
  • Add docstrings
  • Adapt config files
  • Update Docs
  • Make TableLoader load muon information
  • HDF5Source read stuff
  • Make MergeTool merge muon information
  • {x,y} to {fov_lon,fov_lat}

Some (maybe) relavant Issues in this PRs context: #995 #1279
Fixes #1353
Fixes #2033

@StFroese StFroese mentioned this pull request Dec 16, 2022
Open
@maxnoe maxnoe linked an issue Jan 3, 2023 that may be closed by this pull request
Muon processing script does "by-hand" transformation of pixel coords to telescope frame instead of transforming camera geometries #2033
Closed
maxnoe
maxnoe reviewed Jan 10, 2023
View reviewed changes
Comment thread ctapipe/containers.py Outdated
StFroese
StFroese commented Jan 12, 2023
View reviewed changes
Comment thread ctapipe/containers.py Outdated
Comment thread ctapipe/containers.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py
@maxnoe maxnoe added this to the v0.18.0 milestone Jan 13, 2023
nbiederbeck
nbiederbeck reviewed Jan 13, 2023
View reviewed changes
Comment thread ctapipe/conftest.py
Comment thread ctapipe/containers.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
Comment thread ctapipe/image/muon/processor.py Outdated
@RuneDominik RuneDominik mentioned this pull request Jan 13, 2023
Closed
nbiederbeck
nbiederbeck reviewed Jan 19, 2023
View reviewed changes
Comment thread ctapipe/image/muon/processor.py Outdated
maxnoe
maxnoe reviewed Jan 20, 2023
View reviewed changes
Comment thread ctapipe/image/muon/processor.py Outdated
maxnoe
maxnoe reviewed Jan 20, 2023
View reviewed changes
Comment thread ctapipe/image/muon/processor.py Outdated
maxnoe
maxnoe reviewed Jan 20, 2023
View reviewed changes
Comment thread ctapipe/io/hdf5eventsource.py Outdated
maxnoe
maxnoe reviewed Jan 20, 2023
View reviewed changes
Comment thread ctapipe/io/hdf5eventsource.py
maxnoe
maxnoe reviewed Jan 20, 2023
View reviewed changes
Comment thread ctapipe/io/hdf5eventsource.py
maxnoe
maxnoe previously approved these changes Jan 24, 2023
View reviewed changes
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Jan 24, 2023

Thanks! LGTM now.

Let's get this in finally...

nbiederbeck
nbiederbeck previously approved these changes Jan 25, 2023
View reviewed changes
@RuneDominik RuneDominik dismissed stale reviews from nbiederbeck and maxnoe via 351586e January 26, 2023 10:57
maxnoe
maxnoe previously approved these changes Jan 26, 2023
View reviewed changes
nbiederbeck
nbiederbeck previously approved these changes Jan 26, 2023
View reviewed changes
kosack
kosack requested changes Jan 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Looks great! only minor comments

Comment thread docs/changes/2168.api.rst Outdated
Comment thread ctapipe/tools/process.py
@RuneDominik RuneDominik dismissed stale reviews from nbiederbeck and maxnoe via b2a8f9f January 26, 2023 15:25
@kosack kosack merged commit c1f7ba0 into master Jan 26, 2023
@kosack kosack deleted the muon_data_writer branch January 26, 2023 16:41
This was referenced Jan 27, 2023
Closed
Open
Closed
@maxnoe maxnoe linked an issue Jan 27, 2023 that may be closed by this pull request
merge stage1 and reconstruct-muons #1353
Closed
@maxnoe maxnoe mentioned this pull request Jan 27, 2023
Closed
@RuneDominik RuneDominik mentioned this pull request Feb 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RuneDominik RuneDominik RuneDominik left review comments

@maxnoe maxnoe maxnoe approved these changes

@kosack kosack kosack approved these changes

@nbiederbeck nbiederbeck nbiederbeck approved these changes

Assignees

No one assigned

Labels

API change Breaking change muons

Projects

None yet

Milestone

v0.18.0

6 participants

@StFroese @RuneDominik @maxnoe @kosack @nbiederbeck @henningpt