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

Tidying up Offline Event Display #460

Merged
merged 28 commits into from
May 28, 2021
Merged

Tidying up Offline Event Display #460

merged 28 commits into from
May 28, 2021

Conversation

sophiemiddleton
Copy link
Contributor

@sophiemiddleton sophiemiddleton commented May 24, 2021 •
edited
Loading

NOTE: To be merged after dbrown's latest PR

This PR includes lots of minor tidying of the Offline TEve display:

*Editing class parameters to use underscore
*Added descriptions above all functions to aid user understanding of what s being done
*Cut out lots of "junk" parameters from the MainWindow class
*Added in functionality to add an MCTrajectories at the PT, PS and inside the TS (more work to come on this)
*Added FCL parameter to allow user to select which particles are displayed and an appropriate colour scheme is used

Still to be done:
*Fix for 2D plotting error (this is due to the 2DProjectionInterface not clearing on every event, I am working on fix)
*Colour scheme needs adding to GUI (@NamithaChitrazee is working on this)

Screenshot from 2021-05-19 13-33-14
Screen Shot 2021-05-18 at 4 53 42 PM

@FNALbuild
Copy link
Collaborator

Hi @sophiemiddleton,
You have proposed changes to files in these packages:

  • EventGenerator
  • CommonMC
  • Validation
  • ParticleID
  • Analyses
  • TrackCaloMatching
  • /
  • TrackerMC
  • EventMixing
  • TrkFilters
  • TEveEventDisplay
  • CalPatRec
  • ExtinctionMonitorFNAL
  • BFieldTest
  • ConditionsService
  • Filters
  • Mu2eG4
  • CaloCluster
  • TrkExt
  • CRVResponse
  • MCDataProducts
  • fcl
  • JobConfig
  • GlobalConstantsService
  • BFieldGeom

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on master.

The following users requested to be notified about changes to these packages:
@resnegfk

⌛ The following tests have been triggered for 95c4258: build (Build queue has 2 jobs)

About FNALbuild. Code review on Mu2e/Offline.

@kutschke kutschke self-assigned this May 24, 2021
@kutschke
Copy link
Collaborator

Sophie: When you say "Dave's PR" you mean 455, right

I see that you already merged in Dave's branch for 455 into your PR. This creates a situation that I don't think that I have seen before and there is something that I don't understand. Maybe its perfectly OK or maybe it's not - I just don't know. I have added Richie and Andrei as reviewers in the hope that one of them knows the answer.

The issue is that your branch was branched from master 10 PR merges after Dave branch was. So when you merged Dave's branch into yours, did that undo some or all of the intervening ten merges? For the record here is the PR merge history:

Merge pull request #441 from b-vitali/LumiInfo_MR 	      	    ( Sophie - base of PR #460 )
Merge pull request #397 from miscetti/calodmap
Merge pull request #445 from soleti/cosmic_pv
Merge pull request #1 from rlcee/calodmap_names
Merge pull request #450 from bechenard/CaloCrystalDir
Merge pull request #449 from resnegfk/g4study
Merge pull request #448 from resnegfk/g4study
Merge pull request #447 from resnegfk/g4viz
Merge pull request #444 from rlcee/rm_ta
Merge pull request #442 from NamithaChitrazee/master         ( Dave - base of PR #455 )

I have some ideas on how to start answer the question but I hope someone has already encountered this and knows the answer.

@kutschke
Copy link
Collaborator

Ok. That was really dumb on my part. I left in the --dry-run. So it was not supposed to do anything.

Did you try it after removing --dry-run

You can remove the parti file - it's nothing.

@sophiemiddleton
Copy link
Contributor Author

Ok. That was really dumb on my part. I left in the --dry-run. So it was not supposed to do anything.

Did you try it after removing --dry-run

You can remove the parti file - it's nothing.

bash-4.2$ git mv CRVResponse/efficiencyCheck/CRV_Efficiency_check.fcl CRVResponse/test/efficiencyCheck/CRV_Efficiency_check.fcl
fatal: renaming 'CRVResponse/efficiencyCheck/CRV_Efficiency_check.fcl' failed: No such file or directory

@kutschke
Copy link
Collaborator

kutschke commented May 28, 2021 via email

@sophiemiddleton
Copy link
Contributor Author

This worked

@brownd1978
Copy link
Collaborator

brownd1978 commented May 28, 2021 via email

@sophiemiddleton
Copy link
Contributor Author

I can merge in the main branch is we think this is a good idea....let me know what you think

@kutschke
Copy link
Collaborator

kutschke commented May 28, 2021 via email

@sophiemiddleton
Copy link
Contributor Author

Yes, this looks like everything from my point of view

@kutschke
Copy link
Collaborator

kutschke commented May 28, 2021 via email

@brownd1978
Copy link
Collaborator

brownd1978 commented May 28, 2021 via email

@kutschke
Copy link
Collaborator

kutschke commented May 28, 2021 via email

@sophiemiddleton
Copy link
Contributor Author

sophiemiddleton commented May 28, 2021 •
edited
Loading

Ok, I am trying to pull the latest changes but I think I am suffering from the timeout issues seen by others earlier today:

remote_Mu2e     http://github.com/Mu2e/Offline.git (fetch)
remote_Mu2e     http://github.com/Mu2e/Offline.git (push)
bash-4.2$ git pull remote_Mu2e master
fatal: unable to access 'http://github.com/Mu2e/Offline.git/': Failed to connect to github.com port 80: Connection timed out
bash-4.2$ git pull remote_Mu2e main
warning: redirecting to https://github.com/Mu2e/Offline.git/
fatal: couldn't find remote ref main
bash-4.2$

@kutschke
Copy link
Collaborator

kutschke commented May 28, 2021 via email

@sophiemiddleton
Copy link
Contributor Author

Ok, retrying seems to work

@kutschke
Copy link
Collaborator

@FNALbuild run build tests

@FNALbuild
Copy link
Collaborator

⌛ The following tests have been triggered for beb2219: build (Build queue has 2 jobs)

@FNALbuild
Copy link
Collaborator

☀️ The tests passed at beb2219.

Test Result Details
merge ✅ Merged beb2219 at 244a330
scons build (prof) ✅ Log file. Build time: 14 min 47 sec
ceSimReco ✅ Log file
g4test_03MT ✅ Log file
transportOnly ✅ Log file
POT ✅ Log file
g4study ✅ Log file
cosmicSimReco ✅ Log file
ceSteps ✅ Log file
ceDigi ✅ Log file
muDauSteps ✅ Log file
ceMix ✅ Log file
rootOverlaps ✅ Log file
g4surfaceCheck ✅ Log file
FIXME, TODO count 〰️ TODO (4) FIXME (3) in 16 files
clang-tidy 〰️ 30 errors 18 warnings

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@kutschke
Copy link
Collaborator

I will merge this PR now. I have not done a proper look at the code from a software engineering point of view but we do need to do that some time. I did add two issues to the Offline GitHub issue tracker that were motivated by the short look that I did have: #461 and #462. Please keep these in mind has your move ahead.

@kutschke kutschke merged commit c2dc425 into Mu2e:master May 28, 2021
@kutschke kutschke mentioned this pull request May 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonventre bonventre bonventre approved these changes

@kutschke kutschke kutschke approved these changes

@gaponenko gaponenko Awaiting requested review from gaponenko

@brownd1978 brownd1978 Awaiting requested review from brownd1978

Assignees

@kutschke kutschke

Labels
build finished
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sophiemiddleton @FNALbuild @kutschke @bonventre @brownd1978 @sophieMu2e