Skip to content

Comments

Define API for reader and image_mapper#143

Merged
TjarkMiener merged 93 commits intomasterfrom
move_get_image
Feb 11, 2025
Merged

Define API for reader and image_mapper#143
TjarkMiener merged 93 commits intomasterfrom
move_get_image

Conversation

@TjarkMiener
Copy link
Member

This PR suggests APIs for the reader and image_mapper. The configuration system and component scheme of ctapipe is adopted. Besides we adopt astropy tables for the batch generation. Reading is working in monoscopic and stereoscopic mode for DL1 images and R1 waveforms. The code properly process data with different array and telescope (divergent) pointings.

A subclass designed for the adv. trigger system processing R0 waveforms will be added in a separated PR.

–––
Closes #31 #104

For looping over a given dl1 table and a single dl1 event (charges, peak times and mask), we can now retrieve the 2D images (input of the CNNs) without init and running the dl1dh reader.
also create separate function for the trigger patches on R0 data
remove also apply IM functions because it can be now replace by the internal .map_image() function
If prefix camera_frame is in the file, the user should add this prefix to the config file.
added batch generator

removed dl1dh transform

dl1dh provide now a static batch and we get the relevant information about the labels in the data loader of ctlearn
now stored in dl0 monitoring tree
last dimension of sample was missing
replaced by new design
Mainly astropy table operations are now used to retrieve the exmaple identifiers for the stereo reading mode. Code base is therefore heavily reduced and operations are more efficient.

Moved parameter settings outside the dl1dh. User can request to also the read dl1b  parameters by passing a list of column names in the batch_generation()

Removed init skip when pandas hdf5 with example identifiers is provided. It is not needed anymore since we are now fast and efficient with astropy tables and their operations.

split transformation into sub-functions for better readability.
this is removing redundant code

astopy table operations should be used a retrieved sum(), min or max etc.
Everything related to the selection of the subarray is done with ctapipe now

Whenever a new file is processed, it checks the consistency of the SubarrayDescription to the reference which is the first provided file; this ensures that all files have the subarray.
It defines a reading API with two childs for reading in mono and stereo mode. Then, the Image and Waveform childs inherits from both child classes (mono and stereo). Finally, the trigger child only inherits from the mono child
this is somehow needed because we modify the batch Table for the Trigger subclass

fix trigger subclass
@Pablitinho
Copy link
Collaborator

Pablitinho commented Feb 7, 2025

setup.cfg has a deprecated line.
Now:

[tool:pytest]
pep8ignore =

Action (remove pep8ignore line):
[tool:pytest]

If you dont remove this line you will have a warning in pytest like this:

`============================================================================================================================ warnings summary =============================================================================================================================
../../../miniconda3/envs/ctlearn/lib/python3.11/site-packages/_pytest/config/init.py:1441
/Users/pabloguzmansanchez/miniconda3/envs/ctlearn/lib/python3.11/site-packages/_pytest/config/init.py:1441: PytestConfigWarning: Unknown config option: pep8ignore

self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html`

After remove the pep8ignore line , the warning is out.

@Pablitinho
Copy link
Collaborator

Pablitinho commented Feb 7, 2025

Add an empty __init__.py inside "tests" folder and add a minimal test, like this one:

`from dl1_data_handler.reader import DLDataReader

def test_inputs():
dl1dh_reader_type = "DLImageReader"
input_url_signal = ["./asdasd.h5"] # Simulated missing file
input_url_background = []

try:
    reader = DLDataReader.from_name(
        dl1dh_reader_type,
        input_url_signal=sorted(input_url_signal),
        parent=None,
    )
    # If no exception occurs, log a success message
    print("Reader initialized successfully.")
except FileNotFoundError as e:
    print(f"FileNotFoundError caught: {e}")  # Log the error

`
RUN:
$ pip install pytest
$ pytest ./tests

It is working for me.

@TjarkMiener
Copy link
Member Author

I piggybacked on ctapipe for the test data in order to implement a minimal unit test. Can you please check the latest commit @Pablitinho thanks!

process tool was failing in the tests because we need the latest ctapipe version
remove placeholder
dl1dh should be not import keras or tensorflow
@Pablitinho
Copy link
Collaborator

Pablitinho commented Feb 10, 2025

I piggybacked on ctapipe for the test data in order to implement a minimal unit test. Can you please check the latest commit

After check the log. Says that " ModuleNotFoundError: No module named 'tensorflow'" So.. In .github folder you should add tensorflow to be installed and also in environment.yml. After install tensorflow I am getting another error that I am checking "> assert features["input"].shape == (1, 110, 110, 2)
E IndexError: only integers, slices (:), ellipsis (...), numpy.newaxis (None) and integer or boolean arrays are valid indices

dl1_data_handler/tests/test_loader.py:35: IndexError". Let's me take a look

@TjarkMiener
Copy link
Member Author

@Pablitinho thanks for noticing, I just moved the loader class to ctlearn because of the keras/tensorflow dependency. This is solving the tests. I will merge #142 and rebase this branch.

@TjarkMiener
Copy link
Member Author

Thanks a lot @rcervinoucm. CI is passing now with 'minimal' meaningful unit testing. This PR also includes the preparation for dl1dh v0.13.0. Anything missing or can we go ahead? @Pablitinho @nietootein @rcervinoucm

@Pablitinho
Copy link
Collaborator

@TjarkMiener

UnclosedFileWarning:

/Users/pp/miniconda3/envs/ctlearn/lib/python3.11/site-packages/tables/file.py:114: UnclosedFileWarning: Closing remaining open file: /private/var/folders/qk/qydpv_9n4b5g2pl834gyrcw80000gn/T/pytest-of-pabloguzmansanchez/pytest-31/r1_0/gamma.r1.h5 warnings.warn(UnclosedFileWarning(msg)) /Users/pp/miniconda3/envs/ctlearn/lib/python3.11/site-packages/tables/file.py:114: UnclosedFileWarning: Closing remaining open file: /private/var/folders/qk/qydpv_9n4b5g2pl834gyrcw80000gn/T/pytest-of-pabloguzmansanchez/pytest-31/dl1_0/gamma.dl1.h5 warnings.warn(UnclosedFileWarning(msg))

In order to avoid this problem (the files are not closed), the most appropiate solution is the next:

`
...
import atexit

...
class DLDataReader(Component):

def __init__(
    self,
    input_url_signal,
    input_url_background=[],
    config=None,
    parent=None,
    **kwargs,
):
     ....
     atexit.register(self.__destructor)
      ....

def __destructor(self):
    """Destructor to ensure all opened HDF5 files are properly closed."""
    if hasattr(self, "files"):  # Ensure self.files exists before attempting to close
        for file_name in list(self.files.keys()):
            try:
                if self.files[file_name].isopen:  # Check if file is still open
                    print(f"Closing file: {file_name}")
                    self.files[file_name].close()
            except Exception as e:
                print(f"Error closing file {file_name}: {e}")

`
Dont use functions like del or exit . del is called when the user del the object and exit is when is out of the scope.

This is a suggestion, apply it if you wish.

Copy link
Member

@nietootein nietootein left a comment

Choose a reason for hiding this comment

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

Thanks so much, @TjarkMiener and everyone involved!

@TjarkMiener TjarkMiener merged commit e43e32b into master Feb 11, 2025
6 checks passed
@TjarkMiener TjarkMiener deleted the move_get_image branch February 11, 2025 12:01
jbuces pushed a commit that referenced this pull request Mar 13, 2025
Define API for reader and image_mapper
jbuces pushed a commit that referenced this pull request May 7, 2025
Define API for reader and image_mapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use more ctapipe functionality

6 participants