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

refactor: truth tracking examples: fix ODD setup and make default #3270

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Jun 9, 2024

Why?

  • The ODD was never set correctly
  • We usually want to run on the ODD and only have the generic for quick checks.

Note

I put the import from acts.examples.odd import getOpenDataDetector under main, since usually, we want to call the runTruthTrackingFITTERNAME() from outside.

@AJPfleger AJPfleger added the Improvement Changes to an existing feature label Jun 9, 2024
@AJPfleger AJPfleger added this to the next milestone Jun 9, 2024
@github-actions github-actions bot added the Component - Examples Affects the Examples module label Jun 9, 2024
Copy link

codecov bot commented Jun 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.66%. Comparing base (75ef913) to head (1c77639).

Current head 1c77639 differs from pull request most recent head ef2edf2

Please upload reports for the commit ef2edf2 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3270   +/-   ##
=======================================
  Coverage   47.66%   47.66%           
=======================================
  Files         509      509           
  Lines       29425    29425           
  Branches    14131    14131           
=======================================
  Hits        14026    14026           
  Misses       5285     5285           
  Partials    10114    10114           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulgessinger
Copy link
Member

paulgessinger commented Jun 9, 2024

Opinions definitely diverge on this.

One of the primary goals of the Python bindings was originally to get rid of all command line options in favor of a more robust configuration mechanism. I was trying to get the examples be actual examples, rather than flexible command line utilities, where you can open a file, read what it does, and use it.

But I see we've abandoned this approach for at least the ODD example again, where we have dozens of command line options, and therefore two configuration layers on top of each other.

I personally think we're going in the wrong direction here, but I won't try to change everyone's minds.

With the original design, you could have copied the file and called into runTruhTrackingXXX, to reuse common configuration.

@andiwand
Copy link
Contributor

andiwand commented Jun 9, 2024

IMO we could just change this to ODD and have no CLI. I don't see a benefit in switching between generic detector or ODD right now.

It could be a different story if we do physmon on different geometries. We should do that and it might be useful there. But usually one has to change the tracking chain (at least config of algs) to get decent performance and as long as we do not have a generic mechanism of reading a config from somewhere I would also be in favor of having different scripts instead of if all over the place.

@AJPfleger
Copy link
Contributor Author

At least I use the generic detector quite frequently, because it is simpler and faster than the ODD, so I would like to keep it there.

How about the current suggestion? Makes it easily changeable in the python script, avoiding all command line options. And it fixes the ODD setup, that was broken before (the main reason, I made changes).

[title and description need to be change, no need to comment on those.]

@AJPfleger AJPfleger changed the title refactor: add command line options to truth tracking examples refactor: truth tracking examples: fix ODD setup and make default Jun 10, 2024
@kodiakhq kodiakhq bot merged commit 692293b into acts-project:main Jun 10, 2024
52 checks passed
@AJPfleger AJPfleger deleted the truth-tracking-options branch June 10, 2024 12:10
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Jun 10, 2024
@andiwand andiwand modified the milestones: next, v35.2.0 Jun 16, 2024
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
…ts-project#3270)

## Why?
- The ODD was never set correctly
- We usually want to run on the ODD and only have the generic for quick checks.

## Note
I put the import `from acts.examples.odd import getOpenDataDetector` under main, since usually, we want to call the `runTruthTrackingFITTERNAME()` from outside.
timadye pushed a commit to andiwand/acts that referenced this pull request Jun 27, 2024
…ts-project#3270)

## Why?
- The ODD was never set correctly
- We usually want to run on the ODD and only have the generic for quick checks.

## Note
I put the import `from acts.examples.odd import getOpenDataDetector` under main, since usually, we want to call the `runTruthTrackingFITTERNAME()` from outside.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module Fails Athena tests This PR causes a failure in the Athena tests Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants