Merge pull request #153 from cta-observatory/add_pre-commit

Add pre-commit to CI.
cta-observatory · Oct 29, 2023 · c6560fe · c6560fe
2 parents 2337b3d + 236ece5
commit c6560fe
Show file tree

Hide file tree

Showing 61 changed files with 993 additions and 834 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -14,6 +14,19 @@ env:
   PYTEST_ADDOPTS: --color=yes
 
 jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - uses: actions/setup-python@v4
+        with:
+          python-version: 3.8
+      - uses: pre-commit/[email protected]
+        with:
+          extra_args: --files $(git diff origin/main --name-only)
+
   pyflakes:
     runs-on: ubuntu-latest
     steps:

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -0,0 +1,17 @@
+repos:
+  # https://pycqa.github.io/isort/docs/configuration/black_compatibility.html#integration-with-pre-commit
+  - repo: https://github.com/pycqa/isort
+    rev: 5.12.0
+    hooks:
+      - id: isort
+        args: ["--profile", "black", "--filter-files"]
+  - repo: https://github.com/psf/black
+    rev: 22.6.0
+    hooks:
+      - id: black-jupyter
+  # https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html?highlight=other%20tools#flake8
+  - repo: https://github.com/PyCQA/flake8
+    rev: 4.0.1
+    hooks:
+      - id: flake8
+        args: [--max-line-length=88, "--extend-ignore=E203,E712"]
diff --git a/README.md b/README.md
@@ -3,6 +3,7 @@
 Repository for the analysis of MAGIC and MAGIC+LST1 data, based on [*ctapipe*](https://github.com/cta-observatory/ctapipe).
 
 * Code: https://github.com/cta-observatory/magic-cta-pipe
+* Docs (preliminary): https://magic-cta-pipe.readthedocs.io/
 
 v0.3.1 of *magic-cta-pipe* provides all the functionalities to perform a MAGIC+LST-1 or a MAGIC-only analysis. Both types of analyses can be performed using the scripts within the *lst1_magic* folder.
 See the [README](https://github.com/cta-observatory/magic-cta-pipe/blob/master/magicctapipe/scripts/lst1_magic/README.md) for more details on how to run the analysis.
@@ -24,6 +25,8 @@ The following command will set up a conda virtual environment, add the necessary
     conda activate magic-lst1
     pip install .
 
+In general, *magic-cta-pipe* is still in heavy development phase, so expect large changes between different releases.
+
 # Instructions for developers
 
 People who would like to join the development of *magic-cta-pipe*, please contact Alessio Berti (<[email protected]>) to get write access to the repository.
@@ -40,6 +43,14 @@ flake8 magicctapipe     # checks style and code errors
 black filename.py       # reformats filename.py with black
 ```
 
+The *black* and *isort* auto-formatters are used for automatic adherence to the code style. To enforce running these tools whenever you make a commit, setup the [pre-commit hook](https://pre-commit.com/):
+
+```bash
+$ pre-commit install
+```
+
+The pre-commit hook will then execute the tools with the same settings as when the a pull request is checked on github, and if any problems are reported the commit will be rejected. You then have to fix the reported issues before tying to commit again.
+
 In general, if you want to add a new feature or fix a bug, please open a new issue, and then create a new branch to develop the new feature or code the bug fix. You can create an early pull request even if it is not complete yet, you can tag it as "Draft" so that it will not be merged, and other developers can already check it and provide comments. When the code is ready, remove the tag "Draft" and select two people to review the pull request (at the moment the merge is not blocked if no review is performed, but that may change in the future). When the review is complete, the branch will be merged into the main branch.
 
 <!--

diff --git a/docs/conf.py b/docs/conf.py
@@ -3,11 +3,11 @@
 import datetime
 import os
 
-import magicctapipe
-
 # Get configuration information from setup.cfg
 from configparser import ConfigParser
 
+import magicctapipe
+
 setup_cfg = ConfigParser()
 setup_cfg.read([os.path.join(os.path.dirname(__file__), "..", "setup.cfg")])
 setup_metadata = dict(setup_cfg.items("metadata"))
@@ -69,6 +69,7 @@
     "github_url": "https://github.com/cta-observatory/magic-cta-pipe",
     "header_links_before_dropdown": 6,
     "navbar_start": ["navbar-logo", "version-switcher"],
+    "navigation_with_keys": False,
     "switcher": {
         "version_match": version_match,
         "json_url": json_url,
@@ -104,28 +105,28 @@
 # -- General configuration
 
 extensions = [
-    'sphinx.ext.duration',
-    'sphinx.ext.doctest',
-    'sphinx.ext.autodoc',
-    'sphinx.ext.autosummary',
-    'sphinx.ext.intersphinx',
+    "sphinx.ext.duration",
+    "sphinx.ext.doctest",
+    "sphinx.ext.autodoc",
+    "sphinx.ext.autosummary",
+    "sphinx.ext.intersphinx",
 ]
 
 intersphinx_mapping = {
-    'python': ('https://docs.python.org/3/', None),
-    'sphinx': ('https://www.sphinx-doc.org/en/master/', None),
+    "python": ("https://docs.python.org/3/", None),
+    "sphinx": ("https://www.sphinx-doc.org/en/master/", None),
 }
-intersphinx_disabled_domains = ['std']
+intersphinx_disabled_domains = ["std"]
 
-templates_path = ['_templates']
+templates_path = ["_templates"]
 
 # -- Options for HTML output
 
-html_theme = 'pydata_sphinx_theme'
+html_theme = "pydata_sphinx_theme"
 
 # The name for this set of Sphinx documents.  If None, it defaults to
 # "<project> v<release> documentation".
 html_title = f"{project} v{release}"
 
 # -- Options for EPUB output
-epub_show_urls = 'footnote'
+epub_show_urls = "footnote"
diff --git a/magicctapipe/__init__.py b/magicctapipe/__init__.py
@@ -1,16 +1,3 @@
 from .version import __version__
-from . import image
-from . import irfs
-from . import reco
-from . import scripts
-from . import utils
 
-
-__all__ = [
-    "image",
-    "irfs",
-    "reco",
-    "scripts",
-    "utils",
-    "__version__",
-]
+__all__ = ["__version__"]
diff --git a/magicctapipe/conftest.py b/magicctapipe/conftest.py
@@ -1,13 +1,15 @@
-import pytest
+import subprocess
+from math import trunc
+
 import numpy as np
+import pandas as pd
+import pytest
+import yaml
 from astropy.io.misc.hdf5 import write_table_hdf5
 from astropy.table import Table
-import pandas as pd
-import subprocess
-from math import trunc
 from ctapipe.utils.download import download_file_cached
+
 from magicctapipe.utils import resource_file
-import yaml
 
 maxjoint = 13000
 maxmonly = 500
@@ -48,6 +50,7 @@
 Temporary paths
 """
 
+
 @pytest.fixture(scope="session")
 def temp_DL1_gamma(tmp_path_factory):
     return tmp_path_factory.mktemp("DL1_gammas")
@@ -232,10 +235,12 @@ def temp_DL2_real_monly(tmp_path_factory):
 def temp_DL3_monly(tmp_path_factory):
     return tmp_path_factory.mktemp("DL3_monly")
 
+
 """
 Custom data
 """
 
+
 @pytest.fixture(scope="session")
 def dl2_test(temp_DL2_test):
     """
@@ -269,10 +274,12 @@ def pd_test():
     df = pd.DataFrame(np.array([[1, 2], [3, 4], [5, 6]]), columns=["a", "b"])
     return df
 
+
 """
 Remote paths (to download test files)
 """
 
+
 @pytest.fixture(scope="session")
 def base_url():
     return "http://www.magic.iac.es/mcp-testdata"
@@ -283,10 +290,12 @@ def env_prefix():
     # ENVIRONMENT VARIABLES TO BE CREATED
     return "MAGIC_CTA_DATA_"
 
+
 """
 Downloads: files
 """
 
+
 @pytest.fixture(scope="session")
 def dl0_gamma(base_url, env_prefix):
     gamma_dl0 = []
@@ -377,7 +386,7 @@ def config():
 @pytest.fixture(scope="session")
 def config_monly():
     config_path = resource_file("test_config_monly.yaml")
-    return config_path 
+    return config_path
 
 
 @pytest.fixture(scope="session")
@@ -416,6 +425,7 @@ def config_check():
 Data processing
 """
 
+
 @pytest.fixture(scope="session")
 def gamma_l1(temp_DL1_gamma, dl0_gamma, config):
     """

diff --git a/magicctapipe/image/__init__.py b/magicctapipe/image/__init__.py
@@ -1,12 +1,11 @@
+from .calib import calibrate
 from .cleaning import (
     MAGICClean,
     PixelTreatment,
-    get_num_islands_MAGIC,
     clean_image_params,
+    get_num_islands_MAGIC,
 )
 from .leakage import get_leakage
-from .calib import calibrate
-
 
 __all__ = [
     "MAGICClean",

diff --git a/magicctapipe/image/calib.py b/magicctapipe/image/calib.py
@@ -1,37 +1,40 @@
 import numpy as np
-from ctapipe.image import (
-    apply_time_delta_cleaning,   
-    number_of_islands,
-    tailcuts_clean,   
-)
+from ctapipe.image import apply_time_delta_cleaning, number_of_islands, tailcuts_clean
 from ctapipe.instrument import CameraGeometry
 from lstchain.image.cleaning import apply_dynamic_cleaning
 from lstchain.image.modifier import (
     add_noise_in_pixels,
-    random_psf_smearer,  
-    set_numba_seed  
+    random_psf_smearer,
+    set_numba_seed,
 )
-from magicctapipe.image import MAGICClean
 
+from .cleaning import MAGICClean
 
-__all__ = [
-    "calibrate"
-]
+__all__ = ["calibrate"]
 
 
-def calibrate(event, tel_id, config, calibrator, is_lst, obs_id=None, camera_geoms=None, magic_clean=None):
+def calibrate(
+    event,
+    tel_id,
+    config,
+    calibrator,
+    is_lst,
+    obs_id=None,
+    camera_geoms=None,
+    magic_clean=None,
+):
 
     """
-    This function calibrates the camera image for a single event of a telescope 
+    This function calibrates the camera image for a single event of a telescope
 
     Parameters
     ----------
-    event: event 
+    event: event
         From an EventSource
     tel_id: int
-        Telescope ID     
+        Telescope ID
     config: dictionary
-        Parameters for image extraction and calibration    
+        Parameters for image extraction and calibration
     calibrator: CameraCalibrator (ctapipe.calib)
         ctapipe object needed to calibrate the camera
     is_lst: bool
@@ -42,7 +45,7 @@ def calibrate(event, tel_id, config, calibrator, is_lst, obs_id=None, camera_geo
         Camera geometry. Used in case of LST telescope
     magic_clean: dictionary (1 entry per MAGIC telescope)
         Each entry is a MAGICClean object using the telescope camera geometry. Used in case of MAGIC telescope
-    
+
 
     Returns
     -------
@@ -54,23 +57,33 @@ def calibrate(event, tel_id, config, calibrator, is_lst, obs_id=None, camera_geo
         Array of the signal peak time in the camera pixels
 
     """
-    if (not is_lst) and (magic_clean==None):
-        raise ValueError("Check the provided parameters and the telescope type; MAGIC calibration not possible if magic_clean not provided") 
-    if (is_lst) and (obs_id==None):
-        raise ValueError("Check the provided parameters and the telescope type; LST calibration not possible if obs_id not provided") 
-    if (is_lst) and (camera_geoms==None):
-        raise ValueError("Check the provided parameters and the telescope type; LST calibration not possible if gamera_geoms not provided") 
-    if (not is_lst) and (type(magic_clean[tel_id])!=MAGICClean):
-        raise ValueError("Check the provided magic_clean parameter; MAGIC calibration not possible if magic_clean not a dictionary of MAGICClean objects") 
-    if (is_lst) and (type(camera_geoms[tel_id])!=CameraGeometry):
-        raise ValueError("Check the provided camera_geoms parameter; LST calibration not possible if camera_geoms not a dictionary of CameraGeometry objects")
+    if (not is_lst) and (magic_clean is None):
+        raise ValueError(
+            "Check the provided parameters and the telescope type; MAGIC calibration not possible if magic_clean not provided"
+        )
+    if (is_lst) and (obs_id is None):
+        raise ValueError(
+            "Check the provided parameters and the telescope type; LST calibration not possible if obs_id not provided"
+        )
+    if (is_lst) and (camera_geoms is None):
+        raise ValueError(
+            "Check the provided parameters and the telescope type; LST calibration not possible if gamera_geoms not provided"
+        )
+    if (not is_lst) and (type(magic_clean[tel_id]) != MAGICClean):
+        raise ValueError(
+            "Check the provided magic_clean parameter; MAGIC calibration not possible if magic_clean not a dictionary of MAGICClean objects"
+        )
+    if (is_lst) and (type(camera_geoms[tel_id]) != CameraGeometry):
+        raise ValueError(
+            "Check the provided camera_geoms parameter; LST calibration not possible if camera_geoms not a dictionary of CameraGeometry objects"
+        )
 
     calibrator._calibrate_dl0(event, tel_id)
     calibrator._calibrate_dl1(event, tel_id)
 
     image = event.dl1.tel[tel_id].image.astype(np.float64)
     peak_time = event.dl1.tel[tel_id].peak_time.astype(np.float64)
-    
+
     if not is_lst:
         use_charge_correction = config["charge_correction"]["use"]
 
@@ -82,14 +95,14 @@ def calibrate(event, tel_id, config, calibrator, is_lst, obs_id=None, camera_geo
         signal_pixels, image, peak_time = magic_clean[tel_id].clean_image(
             event_image=image, event_pulse_time=peak_time
         )
-            
+
     else:
         increase_nsb = config["increase_nsb"].pop("use")
         increase_psf = config["increase_psf"]["use"]
         use_time_delta_cleaning = config["time_delta_cleaning"].pop("use")
         use_dynamic_cleaning = config["dynamic_cleaning"].pop("use")
         use_only_main_island = config["use_only_main_island"]
-    
+
         if increase_nsb:
             rng = np.random.default_rng(obs_id)
             # Add extra noise in pixels
@@ -133,8 +146,8 @@ def calibrate(event, tel_id, config, calibrator, is_lst, obs_id=None, camera_geo
             max_island_label = np.argmax(n_pixels_on_island)
             signal_pixels[island_labels != max_island_label] = False
 
-        config["increase_nsb"]["use"]=increase_nsb
-        config["time_delta_cleaning"]["use"]=use_time_delta_cleaning
-        config["dynamic_cleaning"]["use"]=use_dynamic_cleaning        
-    
+        config["increase_nsb"]["use"] = increase_nsb
+        config["time_delta_cleaning"]["use"] = use_time_delta_cleaning
+        config["dynamic_cleaning"]["use"] = use_dynamic_cleaning
+
     return signal_pixels, image, peak_time