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

Support for thin type file structure #42

Merged
merged 9 commits into from
Nov 4, 2024
Merged

Conversation

oven8
Copy link
Contributor

@oven8 oven8 commented May 23, 2024

I've added modifications to support thin type file structures. The main differences are noted below:

  1. There are 312 words instead of 273.
  2. Particle data and Cherenkov data contains 8 words (instead of 7) for each particle rsp. with the last one being the weight.

One important change: I had to remove the classmethod decorator over the parse_data_blocks method in the CorsikaFile class.

The code passed the existing pytest modules. I have not added new modules to test for thin binary data files. However I've separately tested with thin data and things seems to be working fine so far. I might modify the test files to add new modules to check for the changes.

corsikaio/file.py Outdated Show resolved Hide resolved
corsikaio/file.py Outdated Show resolved Hide resolved
corsikaio/subblocks/data.py Outdated Show resolved Hide resolved
corsikaio/subblocks/data.py Outdated Show resolved Hide resolved
tests/test_file.py Outdated Show resolved Hide resolved
@maxnoe
Copy link
Member

maxnoe commented May 25, 2024

Hi,

Thanks @oven8, this looks mostly good.

I think we mainly need a smallish test file with the thin option enabled to make sure this keeps working.

Some of the duplications can be removed and the test paths change needs to be reverted.

@oven8
Copy link
Contributor Author

oven8 commented May 25, 2024

Hi,

Thanks @oven8, this looks mostly good.

I think we mainly need a smallish test file with the thin option enabled to make sure this keeps working.

Some of the duplications can be removed and the test paths change needs to be reverted.

I'll add an update with the changes you recommended as well as a small test module.

@oven8
Copy link
Contributor Author

oven8 commented Sep 5, 2024

I have added thin (Cherenkov) binary data file and modified some of the test modules. Let me know if more changes are required.

@maxnoe
Copy link
Member

maxnoe commented Sep 5, 2024

@oven8 This looks mostly good, thanks!

The one concern I have is that the particle test file at 20 MB is quite large for a test file. Could you maybe create a smaller one, the Cherenkov file is fine size-wise!

@oven8
Copy link
Contributor Author

oven8 commented Sep 5, 2024

@oven8 This looks mostly good, thanks!

The one concern I have is that the particle test file at 20 MB is quite large for a test file. Could you maybe create a smaller one, the Cherenkov file is fine size-wise!

I've swapped the thin test file with a smaller one (previously there were 200 events now 5). Let me know if this is okay.

@maxnoe
Copy link
Member

maxnoe commented Nov 4, 2024

@oven8 I am sorry this took so long, looks good now. Feel free to ping me more often next time.

I am going to squash the commits here to make sure the larger test files that were here in intermediate commits don't land in the project history.

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution!

@maxnoe maxnoe merged commit ddfee5a into cta-observatory:main Nov 4, 2024
4 checks passed
maxnoe added a commit that referenced this pull request Nov 4, 2024
Support for thin type file structure
@oven8
Copy link
Contributor Author

oven8 commented Nov 4, 2024

Thanks for all the help! It was nice collaborating with you!

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.

2 participants