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

Resimulation producer #1183

Merged
merged 18 commits into from
Aug 10, 2023
Merged

Resimulation producer #1183

merged 18 commits into from
Aug 10, 2023

Conversation

EinarElen
Copy link
Contributor

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves LDMX-Software/SimCore#49 by adding a new producer that re-simulates events based on the seeds stored in the input file.

Check List

  • I successfully compiled ldmx-sw with my developments

  • I ran my developments and the following shows that they are successful.

I've added an analyzer in the DQM module that compares the sim hits and particles between two passes and tested it with an ecalPN configuration.

  • I attached any sub-module related changes to this PR.

Related Sub-Module PRs

If you have developments in a submodule that you need to merge in along with these developments, please link those PRs here, otherwise just delete this section.
Note: You do not need to make a Sub-Module PR if your submodule developments are already on that repo's default branch.

@tomeichlersmith tomeichlersmith marked this pull request as draft August 9, 2023 14:14
@tomeichlersmith tomeichlersmith marked this pull request as ready for review August 9, 2023 14:15
@tomeichlersmith
Copy link
Member

@EinarElen and I agreed to "hijack" this PR to do some code cleanup and reformatting within SimCore. While the newly introduced ReSimulator is not being tested by these PR Validations - it is helpful to make sure that the broad formatting/refactoring did not change any of the physics. I am not expecting it to since I trust clang to be helpful in this respect, but it is good to double check.

@tomeichlersmith
Copy link
Member

I am getting a python-config error from the DQM module when trying to rerun the validation:

     File "/home/runner/work/ldmx-sw/ldmx-sw/.github/validation_samples/ecal_pn/config.py", line 51, in <module>
      from LDMX.DQM import dqm
    File "/home/runner/work/ldmx-sw/ldmx-sw/install/python/LDMX/DQM/dqm.py", line 23
      DQM/python/dqm.py 
                        ^
  IndentationError: unindent does not match any outer indentation level

Maybe there is an issue with some of the updates you made when writing the DQM resim verifier? I'll leave this to your tomorrow (my tonight) and come back at it tomorrow (mine).

@EinarElen
Copy link
Contributor Author

I am getting a python-config error from the DQM module when trying to rerun the validation:

     File "/home/runner/work/ldmx-sw/ldmx-sw/.github/validation_samples/ecal_pn/config.py", line 51, in <module>
      from LDMX.DQM import dqm
    File "/home/runner/work/ldmx-sw/ldmx-sw/install/python/LDMX/DQM/dqm.py", line 23
      DQM/python/dqm.py 
                        ^
  IndentationError: unindent does not match any outer indentation level

Maybe there is an issue with some of the updates you made when writing the DQM resim verifier? I'll leave this to your tomorrow (my tonight) and come back at it tomorrow (mine).

Hmm, I would more suspect me messing up a rebase here. I'll have a look.

DQM/python/dqm.py Outdated Show resolved Hide resolved
@tomeichlersmith
Copy link
Member

The workflow_dispatch: will only be registered if the workflow on trunk has it.

we can clean up this commit history using a quick rebase before merging
@tomeichlersmith
Copy link
Member

I used my god-like power to add workflow_dispatch: to the workflow on trunk and then put it back here. Then I was able to manually re-trigger the PR Validation run: https://github.com/LDMX-Software/ldmx-sw/actions/runs/5823063957

Unfortunately, it appears like GitHub is not registering that run as associated with this PR which is unfortunate.

@EinarElen
Copy link
Contributor Author

Looks like the validation passed :)

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

looks good to me - a bit worried about conflicts coming from me editing the pr_validation workflow on trunk and this branch but that will be easy for me to patch mid-merge.

@tomeichlersmith tomeichlersmith merged commit 02f20dd into trunk Aug 10, 2023
6 checks passed
@tomeichlersmith tomeichlersmith deleted the SimCore49-ReSim branch August 10, 2023 22:28
@tomeichlersmith
Copy link
Member

(I ended up just doing a non-rebase merge locally

git fetch --all
git switch trunk
git pull
git switch SimCore49-ReSim
git pull
git merge trunk
git switch trunk
git merge SimCore49-ReSim
git push

)

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.

Re-Simulation Processor
2 participants