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

Jump to bottom

Add fuse base plugins to reduce duplicate code #112

Merged
merged 20 commits into from
Feb 1, 2024
Merged

Add fuse base plugins to reduce duplicate code #112

merged 20 commits into from
Feb 1, 2024

Conversation

HenningSE
Copy link
Collaborator

@HenningSE HenningSE commented Jan 25, 2024 •
edited by dachengx
Loading

This PR will introduce fuse base plugins reducing code duplications as suggested by @dachengx in #101.

I still need to figure out how to set the random seed for nest in a smart way when using the base classes.

@HenningSE HenningSE mentioned this pull request Jan 25, 2024
Closed
@HenningSE HenningSE marked this pull request as ready for review January 29, 2024 10:03
@HenningSE
Copy link
Collaborator Author

Test results:

----------------------------------------------------------------------
Ran 21 tests in 519.103s

FAILED (failures=3)

The failing tests are all from the FullChain test:

  • S2PhotonPropagation
  • PMTAfterPulses
  • PulseWindow

They can be related as PMTAfterPulses and PulseWindow depend on the propagated S2 photons.

@dachengx dachengx mentioned this pull request Jan 31, 2024
Merged
dachengx
dachengx reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

Thank you @HenningSE . This looks good, with some comments.

fuse/plugin.py Outdated Show resolved Hide resolved
fuse/plugin.py Show resolved Hide resolved
fuse/plugin.py Outdated Show resolved Hide resolved
fuse/vertical_merger_plugin.py Show resolved Hide resolved
@HenningSE
Copy link
Collaborator Author

Thanks for your comments @dachengx ! I think I implemented all of them!

dachengx
dachengx requested changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

Hey @HenningSE , these are my last comments.

fuse/plugin.py Outdated Show resolved Hide resolved
fuse/plugins/detector_physics/s1_photon_propagation.py Outdated Show resolved Hide resolved
@HenningSE
Copy link
Collaborator Author

We did not bump the version with the last few PRs. I will go to 0.3.0 now as we basically touched every plugin. (Actually it does not really matter as we will go to 1.0.0 hopefully soon)

dachengx
dachengx approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

I understand that the failed tests are because we are still not public.

@dachengx dachengx merged commit 0861489 into main Feb 1, 2024
2 of 4 checks passed
@dachengx dachengx deleted the base_plugin branch February 1, 2024 13:21
This was referenced Feb 1, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dachengx dachengx dachengx approved these changes

@ramirezdiego ramirezdiego Awaiting requested review from ramirezdiego

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HenningSE @dachengx