[GenABEL-dev] [Genabel-commits] r1837 - in pkg/OmicABELnoMM: . examples src tests
L.C. Karssen
lennart at karssen.org
Fri Oct 24 09:28:01 CEST 2014
Hi Alvaro,
On 23-10-14 21:51, Frank, Alvaro Jesus wrote:
> Hi Lennart,
>
> Sorry for the misunderstanding regarding vector. I really rather keep the arrays though.
No problem! That's fine with me. I googled a bit more and it seems there
is a small penalty for std::vector, but more importantly, conversion to
arrays for LAPACK would probably be another place where CPU cycles would
get wasted.
> I will try to break long lines on my next code cleanup pass and do an A-style auto formatting.
> I am focused on the documentation to make sure users can get started. I will try to sit with Yakov and Sodbo to show them the program and its usage (they are in Munich).
>
Excellent! Looking forward to hear about their experience.
By the way, I tried to copy the autotools code for the LaTeX
documentation generation from ProbABEL to OmicABELnoMM and ran into a
bit of an issue. Will try to solve that today and commit.
Lennart.
> Alvaro
> ________________________________________
> From: L.C. Karssen [l.c.karssen at polyomica.com]
> Sent: Thursday, October 23, 2014 6:28 PM
> To: Frank, Alvaro Jesus; genabel-devel at lists.r-forge.r-project.org
> Subject: Re: [GenABEL-dev] [Genabel-commits] r1837 - in pkg/OmicABELnoMM: . examples src tests
>
> Hi Alvaro,
>
> On 21-10-14 18:47, Frank, Alvaro Jesus wrote:
>>
>> The if statements below are a pain to my eyes :-). Due to the formatting
>> (missing indentation and/or missing curly braces) it gives the
>> impression that both the cout and the i = statement are carried out if
>> the condition is true (or maybe even that nothing happens in the
>> if-statement if you take the lack of indentation seriously).
>>
>> At the very least I would indent the cout statement, but given that it
>> is a long line anyway (which are best avoided), I would add {}s and
>> break the cout statement over multiple lines.
>>
>>
>> I will do a code cleanup and address this issues.
>>
>
> That'd be great. Thanks!
>
>>
>>
>> > + if(params.mpi_id == 0)
>> > cout << "Warning, ID names between -g file and interactions file
>> do not coincide! The files must have the same meaningful order!" <<
>> endl;
>> > i = params.n;//exit
>> > }
>> > @@ -433,7 +461,7 @@
>> > }
>> > }
>> > }
>> > - if(interaction_missings)
>>
>> This if statement is indented as it should be, although I would prefer
>> breaking the long line.
>>
>> Editor and screen size dependent, but I can comply.
>
> True, but the fact that it's screen size dependent is one of the reasons
> to stick to a standard width.
>
>
>>
>>
>> Have you ever considered using C++ vectors instead of arrays? They save
>> you the trouble of new/delete, for example. I have no idea if there is a
>> performance hit, however. But I can imagine that combined with iterators
>> they may be well-optimised.
>> Or do you simply use them because LAPACK needs arrays anyway?
>>
>
> If I understand your answer below correctly, it looks like you
> misunderstood what I tried to say. So, before we get into technical
> stuff: in this case I was mostly wondering about whether using
> std::vector would be slower (or faster) than the current use of arrays
> in the case of OmicABELnoMM. See for example
> http://lemire.me/blog/archives/2012/06/20/do-not-waste-time-with-stl-vectors/.
>
> And the last part of my question was: maybe using std::vectors is simply
> not even possible because you need to pass them on to LAPACK routines,
> which accept pointers to arrays, IIRC.
>
> Now going back to what you wrote:
>
>>
>> ISO C++ forbids variable-size array. I cannot comply with every style
>> and avoid all possible warnings at the same time.
>
>
> I don't think I understand your first point. Variable-length arrays
> (VLAs) are indeed not ISO C++ (yet), but that's not what I was after. I
> was wondering if std::vector would be (a lot) slower than your current
> solution. In terms of 'bookkeeping', STL vectors are definitely easier,
> which is why I prefer to use them. I just don't know if they are much
> slower in a program as optimised as OmicABELnoMM.
>
> About the 'bookkeeping':
> Currently you use new to allocate memory on the heap and delete[] for
> deallocation. Using std::vector removes that need. Moreover, if you use
> new to allocate arrays you should actually check whether the allocation
> succeeded.
> As an example of how easy it is to make a mistake, check line 1908 of
> src/AIOwrapper.cpp, where you use a delete that should have been a
> delete[] (thanks to Jenkins' cppcheck for pointing this one out).
>
> As for the C++ standard to use (if that is what you mean with 'style'),
> currently you have -std=c++11 in the default CXXFLAGs in configure.ac.
> I'd stick with that.
>
> About avoiding warnings: Creating warning-free code is paramount IMO,
> because it leads to code that is less prone to bugs and is easier to
> maintain.
>
>
>
>>
>> > Modified: pkg/OmicABELnoMM/src/main.cpp
>> > ===================================================================
>> > --- pkg/OmicABELnoMM/src/main.cpp 2014-10-17 14:57:34 UTC (rev 1836)
>> > +++ pkg/OmicABELnoMM/src/main.cpp 2014-10-21 13:30:17 UTC (rev 1837)
>> > @@ -18,39 +18,41 @@
>> > -d <0.0~1.0> -r <-10.0~1.0> -b -s <0.0~1.0> -e <-10.0~1.0> -i -f";
>> >
>> > string helpcmd_expl =
>>
>>
>> Note that you can use the information generated by autotools for version
>> info. If you include the config.h file that is generated you can do it
>> like we did in ProbABEL, for example in src/usage.cpp:
>>
>> void print_version(void) {
>> cout << PACKAGE
>> << " v. " << PACKAGE_VERSION
>> << "\n(C) Yurii Aulchenko, Lennart C. Karssen, Maarten Kooyman, "
>> << "Maksim Struchalin, The GenABEL team, EMC Rotterdam\n\n";
>> cout << "Using EIGEN version " << EIGEN_WORLD_VERSION
>> << "." << EIGEN_MAJOR_VERSION << "." << EIGEN_MINOR_VERSION
>> << " for matrix operations\n";
>> }
>>
>> The PACKAGE and PACKAGE_VERSION variables comes from config.h, and are
>> generated by autotools. OmicABELnoMM's src/config.h contains them as
>> well.
>>
>>
>> I will try it
>>
>
>
>
> Let me know if it doesn't work out.
>
>
> Best,
>
> Lennart.
>
> --
> *-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*
> L.C. Karssen
> Utrecht
> The Netherlands
>
> lennart at karssen.org
> http://blog.karssen.org
> GPG key ID: A88F554A
> -*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-
>
> _______________________________________________
> genabel-devel mailing list
> genabel-devel at lists.r-forge.r-project.org
> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/genabel-devel
>
--
*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*
L.C. Karssen
Utrecht
The Netherlands
lennart at karssen.org
http://blog.karssen.org
GPG key ID: A88F554A
-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: OpenPGP digital signature
URL: <http://lists.r-forge.r-project.org/pipermail/genabel-devel/attachments/20141024/260b93e6/attachment.sig>
More information about the genabel-devel
mailing list