Code cleanliness improvements in follow-up to #31722 #33865

ericcano · 2021-05-27T12:57:54Z

PR description:

CMSSW #31722 Patatrack integration - Pixel track reconstruction (10/N) was merged with extra comments to be addressed.

This PR addresses the code cleanliness related comments.

PR validation:

The code was run in workflows (step3 and step4):

11634.501 (CPU)
11634.502 (GPU)
11634.505 (CPU)
11634.506 (GPU)

The workflow results where then compared using the plots generated by makeTrackValidationPlots.py. The CPU workflows gave results identical to the baseline integration build. The GPU workflows displayed slight variations, but the variations are also present between multiple runs of the same version (baseline or this branch).

Unit tests were fine except one problem not related the this branch and followed up in #33797

cms-sw#31722 (review)

…sw#614) Used accessors in all cases. This commit does not change the memory layout of the class. Also fixed C-style casts (reinterpret casts are needed here). cms-sw#31722 (comment) cms-sw#31722 (comment)

cms-sw#31722 (comment)

cms-sw#31722 (comment).

cms-sw#31722 (comment)

Also made the type explicit. cms-patatrack#615 (comment)

cms-sw#31722 (comment)

cmsbuild · 2021-05-27T13:04:44Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33865/22895

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoPixelVertexing/PixelTrackFitting/interface/BrokenLine.h modified in PR(s): Code performance improvements in follow-up to #31722 #33864

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33865/22895/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33865/22895/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-05-27T13:54:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33865/22898

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoPixelVertexing/PixelTrackFitting/interface/BrokenLine.h modified in PR(s): Code performance improvements in follow-up to #31722 #33864

cmsbuild · 2021-05-27T13:54:54Z

A new Pull Request was created by @ericcano (Eric Cano) for master.

It involves the following packages:

CUDADataFormats/Track
RecoPixelVertexing/PixelTrackFitting
RecoPixelVertexing/PixelTriplets
RecoTracker/TkSeedGenerator

@perrotta, @makortel, @slava77, @cmsbuild, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @gpetruc, @ebrondol, @mtosi, @dgulhan this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

VinInn · 2021-05-27T14:13:15Z

CUDADataFormats/Track/interface/TrackSoAHeterogeneousT.h

-#ifndef CUDADataFormats_Track_TrackHeterogeneousT_H
-#define CUDADataFormats_Track_TrackHeterogeneousT_H
+#ifndef CUDADataFormats_Track_PixelTrackHeterogeneousT_h
+#define CUDADataFormats_Track_PixelTrackHeterogeneousT_h


why PIXEL?
is not specific to pixel

Indeed, this is a mix-up.

VinInn · 2021-05-27T14:13:47Z

CUDADataFormats/Track/interface/TrackSoAHeterogeneousT.h

+  constexpr auto chi2(int32_t i) const { return chi2_(i); }
+
+  // stateAtBS accessors
+  constexpr TrajectoryStateSoAT<S> &stateAtBS() { return stateAtBS_; }


already discussed. I strongly disagree

This was the consequence of applying the coding rule 4.25 which requires only one of each public, protected and private section. (It can be waived under special circumstances, though).

It is not a question of "special circumstances". Coding rules were written at a time of naive OOD. This is DDD. This is a struct composed of SoAs. A new type of beast. Old rules do not apply.

This was the consequence of applying the coding rule 4.25 which requires only one of each public, protected and private section. (It can be waived under special circumstances, though).

4.25 is Each class may have only one each of public, protected, and private sections, which must be declared in that order.

was it something from section 7 instead?

Yes, also. Rule 7.4 stipulates that structs should be real PODs with no getters and setters, and that applied here.

Never understood why a POD looses its "podness" with trivial getters or setters:
what's the difference btw constepxr float A::x() const; and constexpr float x(A const&); ?
Different are constructors or destructor that definitively defy Podness.

To me 7.4 (and in particular the linked C.131 from the C++ Core Guidelines) does not instruct against a struct with public data members and non-setter/getter member functions.

Never understood why a POD looses its "podness" with trivial getters or setters:

With C++ standard definitionsof POD (or now TrivialType) "podness" is not lost with regular member functions.

Ok. So no reason CMS shall be more restrictive than Standard.

VinInn · 2021-05-27T14:14:48Z

CUDADataFormats/Track/interface/TrajectoryStateSoAT.h

@@ -25,12 +25,12 @@ struct TrajectoryStateSoAT {
    auto cov = covariance(i);
    cov(0) = ccov(0, 0);
    cov(1) = ccov(0, 1);
-    cov(2) = b * float(ccov(0, 2));
+    cov(2) = b * static_cast<float>(ccov(0, 2));


sorry, do not see the reason to add static_cast

This is purely switching to C++ syntax for the cast.

just adding useless characters. I claim that for intrinsic type "C-type cast" is more readable and more appropriate.
There is NO other type of cast that can be applied here. The semantic MUST be the of "C-type cast". Not of any C++ cast.

Is there any use case where it should translate to a reinterpret_cast? There is no dynamic cast for sure with intrisic types and no const correctness issue with an rvalue.

Sorry, reinterpret_cast between double and float is invalid.

just adding useless characters. I claim that for intrinsic type "C-type cast" is more readable and more appropriate.
There is NO other type of cast that can be applied here. The semantic MUST be the of "C-type cast". Not of any C++ cast.

I propose to get an exception in the rule clarified after some discussion in the core group.
@cms-sw/core-l2

The rule 4.15 already allows exceptions, and admittedly explicit narrowing conversions within floating point types falls on the low-risk side of C-style casts.

The C++ Core Guidelines advocates for gsl::narrow_cast (from the guidelines support library). While that describes the intent well, it wouldn't help towards "adding useless characters".

VinInn · 2021-05-27T14:16:00Z

RecoPixelVertexing/PixelTrackFitting/plugins/PixelTrackProducer.cc

-  // store tracks
-  storeTracks(ev, tracks, *httopo);
-}
+DEFINE_FWK_MODULE(PixelTrackProducer);


I already said I was takling care of this in my PR that is coming soon

I can keep this PR on hold and apply on top of yours when it is in.

I think that this PR should be closed. None of the modification in here is neither essential nor an improvement.

VinInn · 2021-05-27T14:16:43Z

RecoPixelVertexing/PixelTrackFitting/plugins/PixelTrackProducerFromSoA.cc

@@ -133,8 +133,8 @@ void PixelTrackProducerFromSoA::produce(edm::StreamID streamID,
  const auto &tsoa = *iEvent.get(tokenTrack_);

  auto const *quality = tsoa.qualityData();
-  auto const &fit = tsoa.stateAtBS;
-  auto const &hitIndices = tsoa.hitIndices;
+  auto const &fit = tsoa.stateAtBS();


as noted before, (and discussed already also in CORE, there is no agreement that this is a better syntax for SOA.

This was also a consequence of the 4.25 rule which forces to choose between direct member access in struct style or using accessors, and this class was a in-between.

There are very good reason why some members are directly accessed and other through a get functon. For instance it let user to think was is going on! It avoids obfuscations.

I think the question meant: is there any chance we want this to ever be a reinterpret_cast ?
If not, then using a static_cast is more accurate than a C cast (that can apply all of static_cast, reinterpret_cast and const_cast at once).

in that specific case we want the compiler to emit a conversion (by value!). We want a temporary float created there!
equivalent to float tmp = f; no more, no less. nothing fancy. As in good old C code (or assembler).
I prefer to introduce such an intermediate statement than use a C++ cast that has not place in math expressions.

The only reinterpret_cast accepted is reinterpret_cast<long long int&>(x) and this is funny as it is equivalent to type punning that is undef behavior. in C++20 they have introduced (at last, after having made undef behavior all C-style idioms!) bit_cast that is equivalent to a memcpy which is the correct semantic. In short nobody uses reinterpret_cast among intrinsic type.

VinInn · 2021-05-27T14:19:25Z

-1
We already discussed that this is NOT the time for all these changes in orthography.
In particular changes in the interface of POD and SoA are not based on any agreed guideline (and this was discussed and agreed in CORE)

slava77 · 2021-06-07T13:59:28Z

@ericcano
There is a "-1" from @VinInn , which is apparently for a part of the changes.
Is there a fraction of changes that can still go ahead?
please clarify your plans for this PR.
Thank you.

ericcano · 2021-06-08T09:16:24Z

@slava77 , I propose to leave the 2 PRs on hold while @VinInn is working on code modifications and re-cherry-pick the syntactic fixes on top.

slava77 · 2021-06-18T13:34:16Z

@slava77 , I propose to leave the 2 PRs on hold while @VinInn is working on code modifications and re-cherry-pick the syntactic fixes on top.

#33889 was merged; now we have a conflict here.
So, it looks like the uncontroversial changes from this and 33864 can now go ahead

fwyzard

I had a look this morning, and I'm not sure there are (m)any uncontroversial changes left...

fwyzard · 2021-06-18T08:24:42Z

CUDADataFormats/Track/interface/TrackSoAHeterogeneousT.h

+#ifndef CUDADataFormats_Track_PixelTrackHeterogeneousT_h
+#define CUDADataFormats_Track_PixelTrackHeterogeneousT_h


Suggested change

#ifndef CUDADataFormats_Track_PixelTrackHeterogeneousT_h

#define CUDADataFormats_Track_PixelTrackHeterogeneousT_h

#ifndef CUDADataFormats_Track_interface_TrackSoAHeterogeneousT_h

#define CUDADataFormats_Track_interface_TrackSoAHeterogeneousT_h

fwyzard · 2021-06-18T08:25:01Z

CUDADataFormats/Track/interface/TrackSoAHeterogeneousT.h

@@ -70,4 +91,4 @@ namespace pixelTrack {

 }  // namespace pixelTrack

-#endif  // CUDADataFormats_Track_TrackHeterogeneousT_H
+#endif  // CUDADataFormats_Track_PixelTrackHeterogeneousT_h


Suggested change

#endif // CUDADataFormats_Track_PixelTrackHeterogeneousT_h

#endif // CUDADataFormats_Track_interface_TrackSoAHeterogeneousT_h

fwyzard · 2021-06-18T08:40:40Z

RecoPixelVertexing/PixelTrackFitting/plugins/PixelTrackProducer.cc

+    produces<reco::TrackExtraCollection>();
+  }
+
+  ~PixelTrackProducer() override {}


Suggested change

~PixelTrackProducer() override {}

~PixelTrackProducer() override = default;

slava77 · 2021-06-24T14:53:21Z

@ericcano
please clarify on the status of this PR.
Thank you.

ericcano · 2021-06-29T12:01:53Z

Given the controversial changes based on coding rules that can be waved and the conflicts with PR #34250, I will close the PR, but keep the underlying issue in patatrack (cms-patatrack#616) open, so we can come back to it after.

ericcano added 9 commits May 26, 2021 14:24

Fixed include guards naming. (cms-sw#614)

07c25b6

cms-sw#31722 (review)

Unified member access methods for class TrackSoAHeterogeneousT. (cms-…

7467e17

…sw#614) Used accessors in all cases. This commit does not change the memory layout of the class. Also fixed C-style casts (reinterpret casts are needed here). cms-sw#31722 (comment) cms-sw#31722 (comment)

Fixed indlude guards naming. (cms-sw#614)

7222e8a

cms-sw#31722 (comment)

Fixed C-style cast. (cms-sw#614)

c52e732

cms-sw#31722 (comment)

Updated comments after methods renaming. (cms-sw#614)

01043c8

cms-sw#31722 (comment).

Modified PixelTrackProducer class to use esConsume(). (cms-sw#614)

e783810

cms-sw#31722 (comment)

Returned complex strcuture by reference instead of value.

323eca7

Also made the type explicit. cms-patatrack#615 (comment)

Fused class, header and SealModule.cc file.

4ed484d

cms-sw#31722 (comment)

Used a wider NaN test to also cover possible Inf with -ofast

0eb287c

cms-sw#31722 (comment)

cmsbuild added this to the CMSSW_12_0_X milestone May 27, 2021

cmsbuild added code-checks-pending heterogeneous-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 27, 2021

cmsbuild mentioned this pull request May 27, 2021

Code performance improvements in follow-up to #31722 #33864

Closed

cmsbuild added code-checks-rejected and removed code-checks-pending labels May 27, 2021

Fixed code format

e1541ce

cmsbuild added code-checks-pending and removed code-checks-rejected labels May 27, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 27, 2021

VinInn reviewed May 27, 2021

View reviewed changes

cmsbuild mentioned this pull request May 29, 2021

Introduce Quality for PixelTracks. Improve Patatrack duplicate & ambiguity resolution #33889

Merged

fwyzard reviewed Jun 18, 2021

View reviewed changes

cmsbuild mentioned this pull request Jun 25, 2021

Speed up Patatrack CA #34250

Merged

ericcano closed this Jun 29, 2021

		#ifndef CUDADataFormats_Track_PixelTrackHeterogeneousT_h
		#define CUDADataFormats_Track_PixelTrackHeterogeneousT_h

	#endif // CUDADataFormats_Track_PixelTrackHeterogeneousT_h
	#endif // CUDADataFormats_Track_interface_TrackSoAHeterogeneousT_h

	~PixelTrackProducer() override {}
	~PixelTrackProducer() override = default;

Code cleanliness improvements in follow-up to #31722 #33865

Code cleanliness improvements in follow-up to #31722 #33865

Conversation

ericcano commented May 27, 2021

PR description:

PR validation:

cmsbuild commented May 27, 2021

cmsbuild commented May 27, 2021

cmsbuild commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

VinInn commented May 27, 2021

slava77 commented Jun 7, 2021

ericcano commented Jun 8, 2021

slava77 commented Jun 18, 2021

fwyzard left a comment

Choose a reason for hiding this comment

fwyzard Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Jun 24, 2021

ericcano commented Jun 29, 2021

fwyzard Jun 8, 2021 •

edited

Loading

VinInn Jun 8, 2021 •

edited

Loading

fwyzard Jun 18, 2021 •

edited

Loading