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.

Sign up for GitHub

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

Propose CEP-4: module structure #2489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Propose CEP-4: module structure #2489

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions docs/developer-guide/ceps/proposed/cep-004-module-structure.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
.. _cep-003:

************************************************************
CEP 4 - Changing the module structure of the ctapipe package
************************************************************

* Status: draft
* Discussion: NA
* Date accepted: NA
* Last revised: 2024-01-08
* Author: Maximilian Linhoff
* Created: 2024-01-08


Abstract
========

``ctapipe``'s module structure has organically grown over its development
in the last couple of years and has some suboptimal properties.

* Large inter-dependencies between modules, sometimes even circular. E.g. importing ``ctapipe.calib`` will import ``ctapipe.image`` because the image extraction code lives in ``ctapipe.image``.
* Deep hierarchies, resulting in issues with references in the sphinx documentation due to the same class being documentated at more than two locations.

A better structure would resolve these issues and would also make it easier to
assign maintainers / code owners to specific submodules.


Proposed new structure
======================

In the new structure, we want to use a flatter hierarchy of smaller modules,
that will resolve the above mentioned issues.

We expect no disadvantages other than:
* Breaking compatibility of imports once
* More namespaces for developers to remember

The second point should be offset by the more logical structure of the namespaces.

.. code::

ctapipe
.atmosphere
Tobychev marked this conversation as resolved.
Show resolved Hide resolved
.camera_calibration
.config
.coordinates
.core
.data_model
.data_volume_reduction
.gain_selection
.image_cleaning
.image_features
.instrument
.muon
.processors
.reconstruction
.tools
.visualization


Looking at it from the current structure, we'd have the following changes:
Copy link
Contributor

Choose a reason for hiding this comment

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

.io and .utlis looks to be missing?

Also, maybe not technically a module, but I wonder what happens to tests/

Copy link
Contributor

Choose a reason for hiding this comment

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

Add .fitting and .exceptions from to the list of top level modules missing from the list


- ``.image`` is split into ``.image_cleaning`` and ``.image_features``
- ``.calib`` is split into ``.camera_calibration``, ``.data_volume_reduction``, ``.gain_selection``
- ``.image.extractor`` is moved to ``.camera_calibration``
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like that image extraction is in camera_calibration: it's not a calibration, but rather a way to reduce noise by integration in the time domain, so it's closer to image cleaning. Calibration should really be limited to things that correct for instrumental differences between simulations and observations. Maybe have a waveform_processing module instead?

Copy link
Contributor

@kosack kosack Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To be more clear:

  • camera calibration should contain things that relate to:
    • DC to PE conversion
    • flat-fielding
    • pedestals
    • noisy pixel detection
    • pointing correction? → maybe in its own module, as this is not camera, but structure related.
  • waveform processing: generation of images from waveforms
    • ImageExtractors
    • related code for peak detection and pulse characterization in general: time over threshold,FWHM, risetime, falltime, etc
    • gain channel selection (or should that be in camera calibration? It's still basically a waveform algorithm looking for an exceeded threshold)

Copy link
Member Author

Choose a reason for hiding this comment

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

pointing correction? → maybe in its own module, as this is not camera, but structure related.

Definitely it's own module, there won't be any code overlap, so it would again lead to a situation where people only wanting to import the pointing correction module would import all the low-level calibrations resulting in large import times.

Copy link
Member Author

Choose a reason for hiding this comment

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

DataVolumeReducers

I added a module specifically for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I noticed that and updated the comment...

- ``.image.muon`` is moved to ``.muon``
- ``.containers`` is renamed to ``.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.

Do you mean core.container.py is moved to its own module?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, containers.py as written

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see!

Then I would add I think it is quite confusing that some of the modules are in their own sub-dirs and others are just files hanging out in the root src/ directory.

I suggest we put everything that is supposed to be a proper module into its own subdirectory, leaving only things like version.py and conftest.py in the root. This also makes a natural place for a dedicated tests/ subdirectory for things like atmosphere and containers instead of having a central one collecting test for various, but not all, modules.

- the configuration related (``Tool``, ``Component``, ``.traits``) parts of ``.core`` are moved to ``.config``
- ``.coordinates``, ``.instrument``, ``.visualization``, ``.reconstruction`` remain as is
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably say that reco is renamed reconstruction.

- The high-level API Components (``ShowerProcessor``, ``ImageProcessor``, ``DataWriter``) are moved to ``.processors``.


Related Issues
==============

* `Importing ctapipe.io is taking very long (>5s) #2476 <https://github.com/cta-observatory/ctapipe/issues/2476>`_
* `Rethink calib and image module scope and coupling #1037 <https://github.com/cta-observatory/ctapipe/issues/1037>`_
* `Move data volume reduction from image to own submodule #1394 <https://github.com/cta-observatory/ctapipe/issues/1394>`_
* `Split-up ctapipe into multiple smaller packages #2090 <https://github.com/cta-observatory/ctapipe/issues/2090>`
Loading