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

Make the material classification numbers an opaque vector #299

Conversation

msmk0
Copy link
Contributor

@msmk0 msmk0 commented Jul 1, 2020

Remove the public enum within the Material class that describes the content of the classification numbers vector. The classification numbers can still be used to stored the encoded material information. In order to interpret it, a Material object has to be constructed again.

The functionality was no used in the code base and removing it will make it easier to change the internal parametrization as planned in #31. Should also fix compiler warning regarding enum name clashes that show up in #267.

@msmk0 msmk0 added Component - Core Affects the Core module Improvement Changes to an existing feature labels Jul 1, 2020
@msmk0 msmk0 added this to the v0.28.00 milestone Jul 1, 2020
@msmk0 msmk0 requested a review from Corentin-Allaire July 1, 2020 11:56
@msmk0 msmk0 changed the title Make the material classification numbers an opaque vector. Make the material classification numbers an opaque vector Jul 1, 2020
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #299 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   48.26%   48.32%   +0.05%     
==========================================
  Files         323      323              
  Lines       16367    16368       +1     
  Branches     7606     7606              
==========================================
+ Hits         7900     7910      +10     
+ Misses       3181     3172       -9     
  Partials     5286     5286              
Impacted Files Coverage Δ
Core/include/Acts/Material/Material.hpp 76.92% <ø> (ø)
Core/src/Material/Material.cpp 93.54% <100.00%> (+30.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8dfa61...37bb2bd. Read the comment docs.

@msmk0 msmk0 requested a review from robertlangenberg July 2, 2020 07:11
@msmk0 msmk0 force-pushed the opaque-material-classification-numbers branch from c1a4149 to 37bb2bd Compare July 2, 2020 07:20
@msmk0 msmk0 removed the request for review from Corentin-Allaire July 2, 2020 07:28
paulgessinger
paulgessinger previously approved these changes Jul 2, 2020
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Testing the approval bot

@paulgessinger paulgessinger removed the Improvement Changes to an existing feature label Jul 2, 2020
@acts-issue-bot acts-issue-bot bot added the Triage label Jul 2, 2020
@paulgessinger paulgessinger added the Improvement Changes to an existing feature label Jul 2, 2020
@acts-issue-bot acts-issue-bot bot removed the Triage label Jul 2, 2020
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the reasoning for moving the enum to outside of the Material class. The title says something is removed and made opaque, but nothing in this PR fits that description.

@paulgessinger
Copy link
Member

Ah nevermind, it's in the .cpp file.

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Ok

@paulgessinger paulgessinger merged commit 8da9475 into acts-project:master Jul 2, 2020
@msmk0 msmk0 deleted the opaque-material-classification-numbers branch July 9, 2020 13:59
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
…ct#299)

Remove the public `enum` within the `Material` class that describes the content of the classification numbers vector. The classification numbers can still be used to stored the encoded material information. In order to interpret it, a `Material` object has to be constructed again.

The functionality was no used in the code base and removing it will make it easier to change the internal parametrization as planned in acts-project#31. Should also fix compiler warning regarding enum name clashes that show up in acts-project#267.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants