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

feat: Add a tool for writing B-fields to disk in CSV format #1470

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

stephenswat
Copy link
Member

@stephenswat stephenswat commented Aug 26, 2022

This PR adds a new example tool, CsvBFieldWriter, that dumps a B-field to disk in CSV format. The tool supports both grid-based and non-grid-based vector fields, it supports Cartesian and symmetrical cylindrical coordinates, and it supports user-specified ranges and bin counts.

Comes with free Python bindings.

@stephenswat stephenswat added Impact - Minor Nuissance bug and/or affects only a single module Component - Examples Affects the Examples module Feature Development to integrate a new feature labels Aug 26, 2022
@stephenswat stephenswat added this to the next milestone Aug 26, 2022
@stephenswat stephenswat self-assigned this Aug 26, 2022
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #1470 (3535486) into main (b032bd2) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 3535486 differs from pull request most recent head c9d82cc. Consider uploading reports for the commit c9d82cc to get more accurate results

@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
+ Coverage   48.46%   48.54%   +0.07%     
==========================================
  Files         384      381       -3     
  Lines       21071    20785     -286     
  Branches     9702     9536     -166     
==========================================
- Hits        10213    10090     -123     
+ Misses       4130     4112      -18     
+ Partials     6728     6583     -145     
Impacted Files Coverage Δ
Core/src/Surfaces/RectangleBounds.cpp 33.33% <0.00%> (-53.34%) ⬇️
Core/include/Acts/Surfaces/Surface.hpp 33.33% <0.00%> (-33.34%) ⬇️
Core/src/Definitions/Common.cpp 0.00% <0.00%> (-32.00%) ⬇️
Core/src/EventData/VectorMultiTrajectory.cpp 66.85% <0.00%> (-10.35%) ⬇️
Core/src/Utilities/Logger.cpp 30.43% <0.00%> (-8.70%) ⬇️
Core/include/Acts/EventData/MeasurementHelpers.hpp 80.00% <0.00%> (-5.72%) ⬇️
...e/include/Acts/EventData/VectorMultiTrajectory.hpp 52.89% <0.00%> (-5.62%) ⬇️
Core/include/Acts/EventData/MultiTrajectory.ipp 64.36% <0.00%> (-2.74%) ⬇️
Core/include/Acts/Propagator/ConstrainedStep.hpp 70.21% <0.00%> (-2.13%) ⬇️
Core/include/Acts/Propagator/Navigator.hpp 54.76% <0.00%> (-0.89%) ⬇️
... and 35 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

looks great 👍

Examples/Io/Csv/src/CsvBFieldWriter.cpp Show resolved Hide resolved
@AJPfleger AJPfleger self-requested a review August 29, 2022 09:17
Examples/Io/Csv/src/CsvBFieldWriter.cpp Outdated Show resolved Hide resolved
Examples/Io/Csv/src/CsvBFieldWriter.cpp Outdated Show resolved Hide resolved
@asalzburger
Copy link
Contributor

Is this good to go? There are still conversations unresolved.

@stephenswat
Copy link
Member Author

stephenswat commented Aug 29, 2022

I think we're waiting for some comments from @timadye.

@stephenswat
Copy link
Member Author

Okay, unless Tim has any comments on the outstanding issue I think this is good to go from my side.

@timadye
Copy link
Contributor

timadye commented Sep 1, 2022

Sorry, I was away. I'll take a look today.

@timadye
Copy link
Contributor

timadye commented Sep 1, 2022

Are you planning to include a CSV reading tool? Apart from providing another good format, that would be useful to test this by writing and reading back.
I see there's a Examples/Detectors/MagneticField/src/FieldMapTextIo.cpp, but that looks like it is space-separated values.

@timadye
Copy link
Contributor

timadye commented Sep 1, 2022

Does this have a different interface compared to Examples/Io/Root/include/ActsExamples/Io/Root/RootBFieldWriter.hpp? It looks like this has the CsvBFieldWriter::CoordinateType coded as a template parameter, rather than an enum Config::gridType. That could be an improvement, but maybe needs considering. If it is better, we might consider changing RootBFieldWriter at a later stage. If so, do we need a more general BFieldWriter::CoordinateType or whatever?

Does this difference follow through to the Python API? It would also be nice to have a Python example and/or test to be sure it works.

Sorry for all the comments.

@stephenswat
Copy link
Member Author

Hi Tim, I do have some changes in the pipeline to the Python B-field writing examples, but I opted to keep them separate from this PR for the time being. I like the idea of having a CSV reader, and I can work on implementing one.

Regarding the coordinate type enum, I agree that it might be useful to merge them, but there is also something to be said for embedding the capabilities of each writer into the class itself.

@paulgessinger
Copy link
Member

@timadye @stephenswat can you advise if we can merge this?

@timadye
Copy link
Contributor

timadye commented Oct 3, 2022

I still had one issue outstanding, here. I hope @stephenswat can check that.

The definitive check would be to read the ATLAS B-field, write with this PR, and check the files have the same binning. If you are not able to do that, I can give it a go.

@stephenswat stephenswat force-pushed the feat/csv_bfield_writer branch 3 times, most recently from 4c35fbd to 8e55fbc Compare October 17, 2022 12:23
@stephenswat
Copy link
Member Author

I have incorporated Tim's patches, and marked him appropriately as a co-author of the commits.

@andiwand
Copy link
Contributor

this conflicts now with #1510

happy to re-approve after the conflict is resolved

@timadye
Copy link
Contributor

timadye commented Oct 27, 2022

I have incorporated Tim's patches, and marked him appropriately as a co-author of the commits.

Thanks @stephenswat! This looks good to me.

I guess you will make the other changes in a separate PR, namely:

  1. harmonize the runXyzGrid() methods with the RootBFieldWriter and others.
  2. replace my ugly dynamic_cast with a static test, eg. if constexpr.

We discussed this in the Tuesday dev meeting and were happy to go ahead with this PR as-is. I could still test the output is correct on the ATLAS B-field. Should I do that now, or wait for you to resolve the conflict with #1510?

@asalzburger
Copy link
Contributor

Hi @stephenswat - can you resolve the conflict here? Then we should be able to get this in.

stephenswat and others added 3 commits November 9, 2022 13:03
New Acts examples tool that dumps a given B-field to CSV format, to
complement the existing dumpers for Root formats.

Co-authored-by: Tim Adye <[email protected]>
This allows us to conveniently use the new CSV B-field writer and dump,
say, a solenoidal B-field for testing.
@stephenswat
Copy link
Member Author

Conflict resolved.

@timadye
Copy link
Contributor

timadye commented Nov 9, 2022

Let me test this now, and then I hope we can merge this sucker.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

📊 Physics performance monitoring for c9d82cc

🟥 ERROR The result has missing elements!
This is likely a physmon job failure

Full report
CKF: ❌ seeded, ❌ truth smeared, ❌ truth estimated
IVF: ❌ seeded, ❌ truth smeared, ❌ truth estimated
❌ Ambiguity resolution
❌ Truth tracking

Vertexing ❌

❌ Vertexing vs. mu

❌ IVF seeded

❌ IVF truth_smeared

❌ IVF truth_estimated

CKF ❌

❌ CKF seeded

❌ CKF truth_smeared

❌ CKF truth_estimated

Ambiguity resolution ❌

❌ seeded

Truth tracking ❌

❌ Truth tracking

Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

I tested with the ATLAS B-field map, and it looks the same as the root input.

@kodiakhq kodiakhq bot merged commit c2de690 into acts-project:main Nov 9, 2022
@github-actions github-actions bot removed the automerge label Nov 9, 2022
@paulgessinger paulgessinger modified the milestones: next, v21.1.0 Nov 11, 2022
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 Feature Development to integrate a new feature Impact - Minor Nuissance bug and/or affects only a single module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants