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

SXP loader #264

Merged
merged 21 commits into from
Nov 18, 2023
Merged

SXP loader #264

merged 21 commits into from
Nov 18, 2023

Conversation

rettigl
Copy link
Member

@rettigl rettigl commented Nov 13, 2023

PR for drafting an SXP loader

@rettigl rettigl mentioned this pull request Nov 13, 2023
@rettigl
Copy link
Member Author

rettigl commented Nov 14, 2023

This should more or less work now, needs to be tested with a full run. For test data upload, I am waiting for David's approval.

@rettigl rettigl force-pushed the sxp_loader_config branch 3 times, most recently from 44f0697 to 498d979 Compare November 15, 2023 21:25
@coveralls
Copy link
Collaborator

coveralls commented Nov 16, 2023

Pull Request Test Coverage Report for Build 6909677854

  • 229 of 287 (79.79%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 89.865%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/loader/test_loaders.py 22 26 84.62%
sed/loader/utils.py 16 21 76.19%
sed/loader/sxp/loader.py 190 239 79.5%
Totals Coverage Status
Change from base Build 6868190392: -0.5%
Covered Lines: 5116
Relevant Lines: 5693

💛 - Coveralls

@rettigl rettigl marked this pull request as ready for review November 17, 2023 13:57
@rettigl
Copy link
Member Author

rettigl commented Nov 17, 2023

This works for now. It has only a few modifications to the flash loader, but probably too many to become a single loader.
The metadata stuff is untouched as of now, this needs to be updated once it's known how it works. Also, the additions from the flash consistency check can be ported once completed, but this I would do in separate PRs.

Copy link
Member

@zain-sohail zain-sohail left a comment

Choose a reason for hiding this comment

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

Generally I have seen the git blame so I got the idea where you made changes.
Is this going to be a permenant loader or it's just for your beamtime?
Because adapting flash loaders structure makes this confusing and likely unmaintainable. We should later definitely think of a solution to this.

Copy link
Member

Choose a reason for hiding this comment

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

Lets wait for data approval before merging, I'd suggest

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sue there will be a public example dataset soon, but I'd still like to merge this and make it usable easily.

@rettigl
Copy link
Member Author

rettigl commented Nov 17, 2023

Generally I have seen the git blame so I got the idea where you made changes. Is this going to be a permenant loader or it's just for your beamtime? Because adapting flash loaders structure makes this confusing and likely unmaintainable. We should later definitely think of a solution to this.

Well, In gerneral the requirements for FLASH and SXP are very similar, i.e. the train /micro/macrobunch structure etc., so it does not make sense to me to deviate too much from what we have. But I agree we need to find a way to maintain this. Either we outsource the common code into utils files, or build an intermedate FEL loader, where FLASH and SXP loader inherit from.

@zain-sohail
Copy link
Member

Generally I have seen the git blame so I got the idea where you made changes. Is this going to be a permenant loader or it's just for your beamtime? Because adapting flash loaders structure makes this confusing and likely unmaintainable. We should later definitely think of a solution to this.

Well, In gerneral the requirements for FLASH and SXP are very similar, i.e. the train /micro/macrobunch structure etc., so it does not make sense to me to deviate too much from what we have. But I agree we need to find a way to maintain this. Either we outsource the common code into utils files, or build an intermedate FEL loader, where FLASH and SXP loader inherit from.

I think best solution is to define different classes for these different aspects and create the actual loaders to instantiate these classes

@rettigl rettigl merged commit 159d1ae into main Nov 18, 2023
@rettigl rettigl deleted the sxp_loader_config branch November 18, 2023 23:43
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.

3 participants