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

Add tutorial for mono dl1b to dl2 and update docs #2691

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

Conversation

LukasBeiske
Copy link
Contributor

@LukasBeiske LukasBeiske commented Jan 24, 2025

Since there are only very minor changes in the code itself, do we need a changelog here?

@LukasBeiske LukasBeiske changed the title Add tutorial for mono dl1b to dl2 and docs Add tutorial for mono dl1b to dl2 and update docs Jan 24, 2025

This comment has been minimized.

@Tobychev
Copy link
Contributor

I think adding an entire tutorial is worth mentioning in the changelog.

This comment has been minimized.

This comment has been minimized.

kosack
kosack previously approved these changes Feb 5, 2025
Copy link
Contributor

@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 good! I wonder if the tutorial should be under tutorials instead of tools, but I think that can wait for a future PR, as there is a lot of documentation cleanup and reorganization that also needs to be done (like actually separating examples and tutorials in the index..).

Also, it looks like you might need to rebase on main since there are branch conflicts.

@maxnoe
Copy link
Member

maxnoe commented Feb 5, 2025

I wonder if the tutorial should be under tutorials instead of tools,

The tutorials section is our section for notebooks generated by sphinx-gallery, not sure if it's possible to add a static rst page there

This comment has been minimized.

@@ -4,6 +4,127 @@
Data Models
***********

.. toctree::
The ``ctapipe`` output files are HDF5 format files, with the following data set hierarchy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Better if you say the data hierarchy is described "below" as that implies some distance, while "following" means something more like "directly after here".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this is the old content of user-guide/data_models/dl1.rst. I did not write any of this, I just fixed the file structure.
I'll have a look again, if I have time.

But this would probably make more sense in a separate PR with a general overhaul of the docs.

.. toctree::
The ``ctapipe`` output files are HDF5 format files, with the following data set hierarchy.
The tables are written with `pytables <https://www.pytables.org>`_ (not ``h5py``).
Containers marked with a ``+`` should be written without their prefix (all others should use column prefixes).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear to me what the purpose of these lines is in the user guide? Could you clarify what you want the user to understand?

- Contents
* - ``subarray``
- event-wise data pertaining to a subarray
- (group)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably explain what "(group)" is supposed to mean, you use with HDF5 terminology without explaining it as far as I can see.

* - Group/Dataset
- Description
- Contents
* - /geometry
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you give the group with a initial "/" but otherwise not typeset in any special way, where before there was no initial slash but the key was set as code. Later you set the groups as code and include an initial "/".

Decide on how you want to present the subgroups and implement it uniformly. I suggest the first style, set as code and without the slash.

- :py:class:`~ctapipe.containers.EventIndexContainer`, :py:class:`~ctapipe.containers.ParticleClassificationContainer`


Simulation Data Model
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not presented as a table when all the other data models are?


* `ctapipe-quickstart <ctapipe.tools.quickstart.QuickStartTool>`: create some default analysis configurations and a working directory
* `ctapipe-process <ctapipe.tools.process.ProcessorTool>`: Process event data in any supported format from R0/R1/DL0 to DL1 or DL2 HDF5 files.
* `ctapipe-apply-models <ctapipe.tools.apply_models.ApplyModels>`: Tool to apply machine learning models in bulk (as opposed to event by event).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more relevant distinction is that this tool applies the models for you, otherwise you would have to make your own script to do it, right? (And in that script you could apply the models however you wish)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should clarify the difference to applying the models using ctapipe-process, so going event-by-event.

docs/user-guide/tools/index.rst Outdated Show resolved Hide resolved

This comment has been minimized.

Tobychev
Tobychev previously approved these changes Feb 6, 2025
Copy link

Passed

Analysis Details

0 Issues

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

Coverage and Duplications

  • Coverage 100.00% Coverage (94.20% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

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.

5 participants