search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.28k forks source link

Check and fix the code for SiPixelTemplate's #20950

Open perrotta opened 6 years ago

perrotta commented 6 years ago

While reviewing PR #20904 the output of the static analyzer pointed out some bad coding in the file RecoLocalTracker/SiPixelRecHits/src/SiPixelTemplate.cc; such a bad coding may likely reflect some possible bug hidden in the code itself, and it is therefore necessary to inspect and address them.

Items spotted by the static analyzer, or revealed by a visual inspection of the code, are the following:

Beside that, I think that a carefull review of the whole code of that class should also be envisaged. For example, the following was found when reviewing PR #22458:

And, more importantly: please always develop the new code basing on its latest version on the cmssw master in github, thus avoinding to inadvertently overwrite other updates possibly integrated between your owns.

@VinInn @makortel @ahinzmann @pmaksim1

perrotta commented 6 years ago

assign reconstruction

cmsbuild commented 6 years ago

A new Issue was created by @perrotta .

@davidlange6, @Dr15Jones, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

cmsbuild commented 6 years ago

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

perrotta commented 6 years ago

@pmaksim1

pmaksim1 commented 6 years ago

Morris Swartz (the author of the code), @ahinzmann and I discussed this in person and via email. While I agree that the present code is atrocious (both messy and possibly confusing), it is correct in a sense that for collisions the cot\beta in the FPIX must always be positive.

So I presume the real issue is then to clean it up? I have another small update (needed to run templates for Phase 2) so we can fold this cleanup with that PR?

pmaksim1 commented 6 years ago

I'd also like to add that, even after the clean-up, indentation, and other beautification, the pixel template code is decidedly expert-only. Its bus factor is just a teeny bit above one. The biggest problem is the code that runs PIXELAV, none of which is in CMSSW.

perrotta commented 6 years ago

@pmaksim1 thank you for looking at it! The biggest open issue was indeed to let a few expert eyes to verify that such a cose was actually doing what it was expected to, and your inspection and evaluation is therefore reassuring. Then, also to avoid confusions and possible mistakes in the future, it also definitely needs to be cleaned up. It is perfectly fine to me if you do so in the PR you are preparing for the Phase2 templates. By the way. when is that PR expected?

perrotta commented 6 years ago

By the way, in the meanwhile I already had ready a couple of trivial cleanings to RecoLocalTracker/SiPixelRecHits/src/PixelCPETemplateReco.cc which allow shoutting up a few warnings of the static analyzer and can save some CPU cycle at the same time.

They are in PR #20968 : if you have nothing in contrary I will sign that PR (as soon as jenkins tests finish) and ask for merging it in 94X. Of course, I did not touch anything else, and I let you clean up and reorganize that code as in your proposal (at least, there are a lot of commented out lines of code and couts that should better be removed...).

perrotta commented 6 years ago

Following the discussion that we had on PR #22458, the following additional issues were found for the code in the package. They are listed here together with the other ones (I am also updating the overall description of this issue), so that they can be fixed all at once when finally the code will get cleaned:

And, more importantly: please always develop the new code basing on its latest version on the cmssw master in github, thus avoinding to inadvertently overwrite other updates possibly integrated between your owns.

@pmaksim1

pmaksim1 commented 6 years ago

@perrotta @slava77 Thanks for the summary: duly noted. Paul and Morris have a proposal for about x4 memory compression. Morris and I are slated to discuss the other miscellaneous code cleanup later in the week.

BTW, the comment "I have no idea what this code is doing, and what it is doing here!" is from me. I wrote the original version of that code, but since then somebody else put these lines in there, without any comments. Why people write code without comments, I have no idea. I don't know how that block passed the code check and was integrated in the first place. If there is GitHub-by way of finding out who committed these lines, that would be great.

boudoul commented 6 years ago

Hi @pmaksim1 , Could it be coming from this PR #3261 ? https://github.com/cms-sw/cmssw/pull/3261/files#diff-f9cb62bd746ece1591e851b06f9e14b0R144

perrotta commented 6 years ago

Petar Maksimovic notifications@github.com ha scritto:

BTW, the comment "I have no idea what this code is doing, and what
it is doing here!" is from me. I wrote the original version of that
code, but since then somebody else put these lines in there, without
any comments. Why people write code without comments, I have no
idea. I don't know how that block passed the code check and was
integrated in the first place. If there is GitHub-by way of finding
out who committed these lines, that would be great.

Hi Petar.

If you open the file in github https://github.com/cms-sw/cmssw/blob/master/RecoLocalTracker/SiPixelRecHits/src/PixelCPEBase.cc you have two buttons on the right: with "Blame" you can see line by
line which commit introduced or modified it last; "History" tells you
the whole history in terms of commits of the file:

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/cms-sw/cmssw/issues/20950#issuecomment-376649075

pmaksim1 commented 6 years ago

@perrotta @boudoul Excellent, thanks! Time for some archaeology!

pmaksim1 commented 6 years ago

I think the code is from Vincenzo. I'll ask him in an email and then add comments explaining what's going on. (What confused me was the mention of strips...)

perrotta commented 6 years ago

Following the discussion in #22911, the following two items were added to the description part which is on top of this issue:

pmaksim1 commented 6 years ago

@perotta @slava77 @schuetzepaul The first one (as I mentioned elsewhere) is fine and can be done quickly.

However, I would like to push back VERY HARD against the second issue. This is a solution in search of a problem. The simulation is supposed to look like the output of RECO. Why shouldn't it depend on RECO? Why is that bad? Is there a formal rule in CMSSW that forbids it?

But even if I were to agree that this is a reasonable request, there is a cost:

1) short-term cost: I recently went through SiPixelRecHits package, and found a bunch of things which needs to be cleaned up. (For instance, we calculate the edge of the cluster three times in three different places.) I was looking at the logic for loading the Lorentz Angle shift from a python config (for Phase2, where it's wrong) and realized that it's about a factor of three more bloated than it needs to be. In a sense, this is natural for a package which ten people patched over ten years, without stopping to pause to throw away the garbage every once in a while. So, I want to do this first.

2) long-term cost: what we are now doing is moving the CORE of pixel RECO code into another package. We'd not only be moving the objects, but the algorithms as well (since they are joint at the hip in this case). So the whole package suddenly becomes twice as hard to manage, since I'd be always checking out two packages rather than one, and making pull requests that involve both. I and others will get confused all the time where the relevant code is. Obviously, it is also going to be harder for anybody new to understand the pixel code since it would be unnaturally and arbitrarily split into two.

I should point out that part of the issues with the last pixel PR was that all relevant files for SiPixel2DTemplateDBObject were spread over several packages. That's why I missed one of the python config files, and that's what made it for me -- a newcomer to this particular aspect -- to navigate. I used LXR in the past month more than in the past five years, simply to keep re-finding things that I've found yesterday, because they were in places that I couldn't remember. (And still don't.)

So: I understand your motivation. But, all considered, I think this is a terrible, terrible idea.

I don't know what's the equivalent of the Supreme Court at CMS, but if this goes forward I am going to fight it until there.

pmaksim1 commented 6 years ago

I rarely get passionate about anything, but this is something that will make my days miserable for years to come, so I hope you will understand why I am getting so revved up.

perrotta commented 6 years ago

@pmaksim1 : the point is that there is some code (namely the definitions of the templates) on which both Reconstruction and Fast/Full Simulation depend upon. That code should be better placed in a package which does not depend on Reco or Sim, so that there will be no cross dependencies among the two systems (now Fast/Full Sim depends on Reco).

From a quick inspection, I think the files that should be moved are only

HOWEVER; I don't plan to get engaged in front of the Supreme Court of CMS ;-) Therefore, if you think that the move above cannot be afforded now, please go ahead with all other improvements, fixes and cleanings of the code according to your plan: we can descope that move to a later time

slava77 commented 6 years ago

On 4/13/18 11:28 PM, perrotta wrote:

@pmaksim1 https://github.com/pmaksim1 : the point is that there is some code (namely the definitions of the templates) on which both Reconstruction and Fast/Full Simulation depend upon. That code should be better placed in a package which does not depend on Reco or Sim, so that there will be no cross dependencies among the two systems (now Fast/Full Sim depends on Reco).

From a quick inspection, I think the files that should be moved are only

  • SiPixelTemplate.h + .cc
  • SiPixelTemplate2D.h + .cc
  • SiPixelTemplateDefs.h Moving just them would eliminate cross-dependencies without overturning the whole structure.

HOWEVER; I don't plan to get engaged in front of the Supreme Court of CMS ;-) Therefore, if you think that the move above cannot be afforded now, please go ahead with all other improvements, fixes and cleanings of the code according to your plan: we can descope that move to a later time

The arguments against moving should perhaps be stronger than just inability to edit files in future developments when these files are in more than one package.

pmaksim1 commented 6 years ago

@perrotta @slava77 Executive summary: I propose that we defer this until later. A couple of follow-ups below:

1) if you browse SiPixelTemplate*.cc, you'll see that this is not just storage, there is a substantial amoun tof non-trivial pixel reco code burried inside: template interpolation, calculation of errors, and, especially, the caculation of Qbin (= classification of the amount of the charge) which determines the behavior of RECO code for this cluster, etc. So if you move RECO code somewhere else, it's still RECO code. You will still have to sign on any simulation PR that touches these files.

2) I still fundamentally don't understand WHY this is bad. Both simulation and reconstruction should depend on the underlying physics of charge deposition and collection in the respective detector. For the pixels that amount of physics is both (a) complicated and (b) largely understood, and that's why there is a lot of code that should underlie simulation and RECO. The separation between simulation and reconstruction is thus somewhat largely artificial and arbitrary. (I don't know how it's done for other detector systems, but I suspect that the physics is simpler and thus can be factorized/duplicated.) This wasn't an issue because the SiPixelDigitizer was grossly simplistic, and we worked around that in analyses by measuring data/MC scale factors.

If simulation and RECO depended on each other, that would be bad. But if simulation depends on RECO, then this should be fine. Factorizing the code would result in splitting RECO into RECO1 and RECO2, so then we would have RECO2 and simulation depending on RECO1, but the fact is that RECO1 is still reco. We can rename RECO1 something like "detector physics", but it's all just a name.

3) Finally, a response to Slava's last point: we do many things because they are better for us humans. AI could write FORTRAN just fine, and doesn't need OOAD to write bug-free, functioning code. We don't use FORTRAN and use C++ with smart pointers because we humans are imperfect, we make mistakes. I am making exactly the same argument: spreading code over many packages is not good. Having dealt with SiPixel2DTemplateDBObject, I am convinced that this was designed quite poorly: it was designed with organizational elegance in mind, even though it was less optimal for people for me and caused me to make mistakes I shouldn't have made if the things were done properly. (And here by "properly" I mean minimizing mistakes, speeding-up development, simplifying the training of newcomers, and ease long-term maintenance; by that metric the DB interface code is a total failure.)

Splitting the SiPixelRecHits makes it 10% harder to keep track of what is where (for me and for people who will be maintaining this in 15 years), and makes everything slower. It will make the mistakes when making a PR ~ 10% more likely, and integrating the same PRs 10% longer. That's neither in my own interest, nor yours (I hope), nor in the interest of CMS.

I'll be happy to talk to the management to make this point clear.

I should point out that you guys are working very hard to keep the code understandable and clean. I am quite impressed by that (and the whole PR system we use now, even though it was quite painful). All I am doing now is to point out that this suggestion is not as constructive as it appears, all things considered...

slava77 commented 6 years ago

the goal of this request to factor out SiPixelTemplate is to reduce package/library level inter-dependencies. It will likely stay in the reco category, indeed.

fabiocos commented 6 years ago

@pmaksim1 I fully support @slava77 and @perrotta request, and after reading this discussion to be honest I am not so much in favour to integrate #22911 as it is, knowing that this will introduce a problem that will not be cleaned.

It is not just a matter of "elegance", it is a matter of having a clean software distribution minimizing interdependencies, that are always a potential source of very practical trouble. The files that Slava mentions are effectively kind of complex data formats, and the fact that now the digitization has to depend on them is the most clear demonstration of this.

Having files distributed in more than on directory is normal in a complex software structure like CMSSW, having to recompile the digitization because the reconstruction changes is not so normal. If these data formats were completely local to the package they belong to, as it was up to now, it would not be an issue. But with #22911 this is no more true, and it was not even true before for FastSim.

The fact that the physics behind simulation and reconstruction is common is a completely irrelevant justification to make different workflows depend one on another, and it even goes in the direction that we are suggesting. Both simulation and reconstruction needs 4-vectors, for this reason we put them into a third package, and do not require Geant4 to be recompiled when we change the particle flow.

As far as I can see, moving these few files into a third package and changing the few headers and BuildFile including them probably requires less work than that was taken by this thread.

pmaksim1 commented 6 years ago

@fabiocos, moving the files into a third (presumably new) package of course is not hard. What is harder is to live with it. I found the situation with the event setup objects to be extremely confusing (and as we've seen, error prone) simply because the stuff is spread over many packages.

The problem is that you are all experts, you know where everything is. To you three (and others who commented in earlier PR threads) this is not a problem. However, this is not my day job... My CMSSW time is extremely fragmented... I can't find anything without LXR, even the stuff I edited yesterday. The code and config files are scattered seemingly at random, and 90% of the time I can't figure out what the underlying philosophy behind the location of the files is.

Our goal should be to make the code a) easier to understand b) easier to navigate c) thus easier to fix d) easier to bring in new people (e.g. students who barely know some C++) to do (a-c)

Every time we involve another package, we increase the probability of mistakes, and we reduce the chance to pull in new manpower -- and even if we do so, we reduce the effectiveness of that manpower. How that is not a relevant argument???

The example of the 4-vectors is a good one, but it is too simplistic. The trouble with pixel templates is that they are not data formats, they are essentially CondFormats with embedded RECO algorithms. Splitting this code essentially splits off a chunk of pixel RECO into CondFormats, and I think that's equally confusing and frankly unappealing. This is like having 4-vectors welded to half a particle flow, so that that half of particle flow would then have to move to the library with 4-vectors. So while we'd gain but not having to link with the other half of particle flow, we'd now have particle flow code split into two halves, the other one is some obscure place in never-never land of 4-vectors. This is neither clean, nor helpful, and thus is less maintainable.

On the other hand, I don't understand what this has to do with workflows? So what if the simulation workflow needs to run reconstruction? The reverse would be unnatural, I agree, but simulation depending fully and completely on reconstruction to me seems does not seem strange. I presume that if the non-template code changed, then the digitization would not be recompiled. It would be compiled if the template header files changed -- but that would happen in both cases. So I'm still unclear what we are gaining. But it's very clear to me what we are losing.

On Thu, Apr 19, 2018 at 12:44 PM, Fabio Cossutti notifications@github.com wrote:

@pmaksim1 https://github.com/pmaksim1 I fully support @slava77 https://github.com/slava77 and @perrotta https://github.com/perrotta request, and after reading this discussion to be honest I am not so much in favour to integrate #22911 https://github.com/cms-sw/cmssw/pull/22911 as it is, knowing that this will introduce a problem that will not be cleaned.

It is not just a matter of "elegance", it is a matter of having a clean software distribution minimizing interdependencies, that are always a potential source of very practical trouble. The files that Slava mentions are effectively kind of complex data formats, and the fact that now the digitization has to depend on them is the most clear demonstration of this.

Having files distributed in more than on directory is normal in a complex software structure like CMSSW, having to recompile the digitization because the reconstruction changes is not so normal. If these data formats were completely local to the package they belong to, as it was up to now, it would not be an issue. But with #22911 https://github.com/cms-sw/cmssw/pull/22911 this is no more true, and it was not even true before for FastSim.

The fact that the physics behind simulation and reconstruction is common is a completely irrelevant justification to make different workflows depend one on another, and it even goes in the direction that we are suggesting. Both simulation and reconstruction needs 4-vectors, for this reason we put them into a third package, and do not require Geant4 to be recompiled when we change the particle flow.

As far as I can see, moving these few files into a third package and changing the few headers and BuildFile including them probably requires less work than that was taken by this thread.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/issues/20950#issuecomment-382803540, or mute the thread https://github.com/notifications/unsubscribe-auth/AEppUUov20QuHJikkLjHQs5ijRLFBKSMks5tqL7rgaJpZM4P9YV1 .

pmaksim1 commented 6 years ago

@fabiocos @perrotta @slava77 So I thought about this some more, and I think we still don't quite understand each other. For example, what does the sentence "it is a matter of having a clean software distribution minimizing interdependencies, that are always a potential source of very practical trouble." exactly means? Does it mean that

a) this will lead to a technical problem, like two libraries that depend on each other, or a library which is loaded and shouldn't?

b) or, that this will confuse people.

What is an example of a "very practical trouble"? I have outlined above several sources of very practical troubles that will arise if we do this, but I'd be happy to give you very detailed scenarios. The number one being that various files will end up copied into wrong places, and @perotta and @slava77 will have more work to do while integrating PRs.

While this whole matter seems silly (since I kept my mouth shut I would have saved everybody a lot of time), I deeply feel that the situation from the non-expert developer's point of view is quite dire, and this will make it 10% more dire. You can say that 10% is a small price to pay for satisfying the abstract principle that simulation does not depend on reco, but it does not feel to me this way.

That is to say: you can order me to do it, but that does not mean that I don't think it's a terrible idea.

davidlange6 commented 6 years ago

On Apr 20, 2018, at 6:32 AM, Petar Maksimovic notifications@github.com wrote:

@fabiocos @perrotta @slava77 So I thought about this some more, and I think we still don't quite understand each other. For example, what does the sentence "it is a matter of having a clean software distribution minimizing interdependencies, that are always a potential source of very practical trouble." exactly means? Does it mean that

a) this will lead to a technical problem, like two libraries that depend on each other, or a library which is loaded and shouldn't?

b) or, that this will confuse people.

My answer: both

Looking through this code - indeed SiPixelTemplate is a classic CMS CondFormat class with a bit more code than normal inside. But then there is a sort of transient to persistent conversion that is more a la Atlas than a la CMS implemented as SiPixelTemplateDBObject. [but it facilitates schema evolution in this case, which is perhaps required] - this allowed SiPixelTemplate to get developed in RecoLocalTracker.

The idea of separating data format like objects from algorithms is there at a low level in CMSSW and when followed really helps simplify the dependency structure of everything.

I agree the code is currently in a bunch of places making it difficult to develop and confusing to people that happen into this area. Examples are PixelCPEClusterRepairESProducer or PixelCPETemplateRecoESProducer in RecoLocalTracker/SiPixelRecHits/ rather than the more obvious CalibTracker/SiPixelESProducers.

We had a similar case with the hcal darkening code a last year or so. After a few problems with circular dependencies and changes that were difficult to understand the effects of, code got rearranged. The resolution was the same as suggested in this case and has worked quite well since then.

What is an example of a "very practical trouble"? I have outlined above several sources of very practical troubles that will arise if we do this, but I'd be happy to give you very detailed scenarios. The number one being that various files will end up copied into wrong places, and @perotta and @slava77 will have more work to do while integrating PRs.

While this whole matter seems silly (since I kept my mouth shut I would have saved everybody a lot of time), I deeply feel that the situation from the non-expert developer's point of view is quite dire, and this will make it 10% more dire. You can say that 10% is a small price to pay for satisfying the abstract principle that simulation does not depend on reco, but it does not feel to me this way.

That is to say: you can order me to do it, but that does not mean that I don't think it's a terrible idea.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fabiocos commented 6 years ago

@davidlange6 thanks for the comment. I think that a code cleaning could be done possibly in steps, but avoiding the dependency introduced by #22911 is likely the first of them.

pmaksim1 commented 6 years ago

@davidlange @fabiocos

I talked to Morris yesterday about all this, and he urged me to be reasonable and to accept this decision since that would be the only way to move things forward. So I'll do this, but I warn you that this doesn't really solve the problem since the dependencies are real and moving code around won't change anything.

Thus my proposal is this:

1) integrate #22911 now 2) Morris is cleaning up his code so this can go next 3) pixel FastSim PR goes next. (I wanted to make the PR in February but I wanted ClusterRepair first. I think that plan backfired.) 4) clean up SiPixelRecHits (delete unneeded code, avoid duplication, identify which code is needed by FastSim), make another PR 5) finally, make a new package, and move SiPixelTemplate stuff plus all other RECO code needed by FastSim into another package; at that point, SiPixelRecHits keeps: (a) template fit (b) CPE generic formula (c) CPE shells (the ES crap) and (d) RecHit ED producer.

Does this work?

I am sure I will regret this, and most likely you will too.

On Sat, Apr 21, 2018 at 7:41 AM, Fabio Cossutti notifications@github.com wrote:

@davidlange6 https://github.com/davidlange6 thanks for the comment. I think that a code cleaning could be done possibly in steps, but avoiding the dependency introduced by #22911 https://github.com/cms-sw/cmssw/pull/22911 is likely the first of them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/issues/20950#issuecomment-383288749, or mute the thread https://github.com/notifications/unsubscribe-auth/AEppUfqD6G5oPT89dnq69arAxybK5Iimks5tqxrdgaJpZM4P9YV1 .

pmaksim1 commented 6 years ago

@fabiocos @davidlange @perrotta @slava77

If you want CMS to have a better full simulation, and especially the simulation of radiation damage in pixel sensors, then @schuetzepaul 's PR (#22911) should go in asap.

I also would like to get in some feedback to the above plan. The memory improvement can go in at any time, so we can either pull the lever on that pull request immediately, or it could wait a week and then it can also pull in other cosmetic changes in Morris's code. Let us know.

fabiocos commented 6 years ago

@pmaksim1 @schuetzepaul this PR cannot be used in a full production environment until a production release containing it is produced, and the normal timescale for the build of 10_2_0 is end of June. Of course dedicated tests could be done in pre-releases, but in general this is for limited size samples. Could you please clarify the timescale of the software reorganization plan sketched above?

pmaksim1 commented 6 years ago

@fabiocos

Just talked to Morris, so here are realistic time estimates:

Thus my proposal is this:

1) integrate #22911 now

2) Morris is cleaning up his code so this can go next

This breaks down into two parts:

2a) Reducing memory footprint of 2D templates -- READY (I can make a PR within 24 hrs)

2b) cosmetic cleanup.

As you probably know, the annealing of Layer 1 was larger than we thought, so Morris is recalibrating the current detector, so that we can take data. That's the highest priority. The cosmetic cleanup is in progress, but won't be done until 2-3 weeks from now.

3) pixel FastSim PR goes next. (I wanted to make the PR in February but I wanted ClusterRepair first. I think that plan backfired.)

I'd say it's a couple of days if I am not doing any other PR. Say a week, just to make it safe. (I need to re-test it in a new release.)

4) clean up SiPixelRecHits (delete unneeded code, avoid duplication, identify which code is needed by FastSim), make another PR

Say a month. I'll clean-up as I go along, so this can be folded into (3) or (2b) as well.

5) finally, make a new package, and move SiPixelTemplate stuff plus all other RECO code needed by FastSim into another package; at that point, SiPixelRecHits keeps: (a) template fit (b) CPE generic formula (c) CPE shells (the ES crap) and (d) RecHit ED producer.

We could fold this into the previous PRs, if you want to have fewer PRs.

But I think this will be safer to do separately, simply to reduce the chance of mistakes. I can only juggle so many balls, and I think plugging in Morris's updates + fixing FastSim + moving low-level code around + moving big stuff into new packages + beautifying everything = too much for me to handle for one PR.

So if this were up to me, I'd give this two months and do it when I get back from vacation at the end of June.

Thanks!

pmaksim1 commented 6 years ago

I also would like to make an appeal for integrating things in smaller chunks, since

  1. You can see what goes in more easily. (Less code to wade through.)
  2. All the integration tests are incredibly useful. (I'm a big fan of adiabatic changes.)
  3. Other people can develop code in the meantime; since few normal people know how to rebase properly (at least I don't), I think this will help others contribute. I find the development based on other people's merge topics quite painful and error prone, so it would be nice to avoid that.
  4. Thus, the development and testing by others (especially students who we can employ to run jobs and make plots) can be facilitated if we have this in a real release, even just an integration build. It will reduce errors that arise from following a complicated recipe by merging several topics on top of each other. (At least I don't myself to do this properly, and thus I don't trust others as well.)
fabiocos commented 6 years ago

After a direct discussion, we agreed that the plan might converge for 10_2_X. For this reason I integrate #22911 , leaving for some period with this dependency, in view of a global cleaning of the code.

pmaksim1 commented 6 years ago

@slava77 @perrotta @boudoul @veszpv @tvami @tsusa @schuetzepaul @ssekmen

Hi everybody, @fabiocos and I talked just now and agreed on the following plan: 1) integrating #22911 (as Fabio mentioned above) 2) 2D template memory improvements + bug fixes + cosmetic cleanup = new PR this week (Wed or more likely Thu) 3) FastSim for pixels = another new and independend PR (Fri or next weekend) 4) further code cleanup (consolidating helper functions in a separate object) + moving that object and templates into a separate package = try to make the PR by end of May so that we can test it and have it hopefully integrated by mid-June. Note that this relies on (2) and (3) PRs to be already integrated. (And hence I'd like to defer non-critical subsequent cleanup for the third PR.)

We will try hard to do all this in time for the end-of-June release.