pegeler commented 4 years ago

Hi @RobinHankin,

Thanks again for the great package. It was a lot of fun to read through your code!

Here's the rundown of proposed changes:

Fixed issue #9 by returning integer(0) for all zero-length objects passed into conjugate.
Fixed issue #11 by doing bounds-checking on the array.
The conjugate function accepts data in non-standard form by adding a very simple counting sort function. I could presort using base::sort before sending down into C, but did not because this was faster and I figure we can make a lot of assumptions about the types of data being fed into this function.
The durfee function accepts data in non-standard form by importing my eddington package and using E_num.

Both durfee and conjugate behave as they did by default. If the user wants to feed in unsorted data, they will need to specify that using the sorted flag.

Please feel free to give me constructive criticism. I'm always open to suggestions and looking for ways to improve.

Paul

codecov[bot] commented 4 years ago

Codecov Report

Merging #12 into master will increase coverage by 2.61%. The diff coverage is 96.61%.

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   74.14%   76.76%   +2.61%     
==========================================
  Files           6        6              
  Lines         704      736      +32     
==========================================
+ Hits          522      565      +43     
+ Misses        182      171      -11

Impacted Files	Coverage Δ
src/partitions_init.c	`100.00% <ø> (ø)`
src/partitions.c	`98.66% <95.74%> (+2.20%)`	:arrow_up:
R/partitions.R	`72.68% <100.00%> (+2.06%)`	:arrow_up:

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 926b21d...05d1aa5. Read the comment docs.

pegeler commented 4 years ago

Hello @RobinHankin,

Thanks for getting back with me so quickly. In this case, the compiler was being exceedingly helpful. I mixed up the order of operands for the calloc function, which I tend to do if I don't have the man page open in front of me. My compiler didn't catch it while rebuilding the package, so I'm glad you found the issue. I apologize for the error!

Commit 21842c5 fixes the issue. You should be good to go.

Thanks,

Paul

RobinHankin commented 4 years ago

hi again, thanks for this. I've run the latest partitions_1.10-0.tar.gz through a local R CMD check and get this:

rhankin@cuttlefish:~/Downloads $ R CMD check partitions_1.10-0.tar.gz 
* using log directory ‘/home/rhankin/Downloads/partitions.Rcheck’
* using R version 4.0.2 (2020-06-22)

[snip]

* checking loading without being on the library search path ... OK
* checking dependencies in R code ... NOTE
Namespace in Imports field not imported from: ‘mathjaxr’
  All declared Imports should be used.
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK

[snip]

* checking re-building of vignette outputs ... OK
* checking PDF version of manual ... OK
* DONE

Status: 1 NOTE
See
  ‘/home/rhankin/Downloads/partitions.Rcheck/00check.log’
for details.

I am confused by this as winbuilder didn't report a similar NOTE, and I've struggled with mathjaxr and DESCRIPTION files in the past. The mathjaxr package README says that it should be placed under either Suggests: or Imports: but I have received not entirely reproducible QC warnings when it is listed under Imports: which is why it is under Suggests: in my other packages (e.g. the clifford package, which is currently driving me nuts [for unrelated reasons]). Best wishes, Robin

pegeler commented 4 years ago

Interesting to find yet another platform-specific problem. It appears this one is unique to MacOS. The fix is pretty simple; I just added import("mathjaxr") to the NAMESPACE file. See commit 4cc1770.

I've run R CMD check on MacOS Catalina with R 3.6.3 and did not get the NOTE. Please let me know if you find anything else. I am very glad to help.

RobinHankin commented 4 years ago

Hi there, sorry about this but winbuilder gives the following report (which I thought was the same issue as before, but it's not). Again, I'm not sure if the QC is being needlessly over-zealous or whether it's a genuine warning. In any event, here it is:

* installing *source* package 'partitions' ...
** using staged installation
** libs

*** arch - i386
d:/Compiler/rtools40/mingw32/bin/g++ -std=gnu++11  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -c app.cpp -o app.o
d:/Compiler/rtools40/mingw32/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c partitions.c -o partitions.o
In function 'c_sort.part.0',
    inlined from 'c_sort' at partitions.c:263:6:
partitions.c:268:20: warning: argument 1 value '2147483649' exceeds maximum object size 2147483647 [-Walloc-size-larger-than=]
   int *a = (int *) calloc(max + 1, sizeof(int));
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from partitions.c:2:
partitions.c: In function 'c_sort':
D:/Compiler/rtools40/mingw32/i686-w64-mingw32/include/stdlib.h:455:17: note: in a call to allocation function 'calloc' declared here
   void *__cdecl calloc(size_t _NumOfElements,size_t _SizeOfElements);
                 ^~~~~~
d:/Compiler/rtools40/mingw32/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c partitions_init.c -o partitions_init.o
d:/Compiler/rtools40/mingw32/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c permutations.c -o permutations.o
d:/Compiler/rtools40/mingw32/bin/g++ -std=gnu++11 -shared -s -static-libgcc -o partitions.dll tmp.def app.o partitions.o partitions_init.o permutations.o -Ld:/Compiler/gcc-4.9.3/local330/lib/i386 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R-4.0.2/bin/i386 -lR
installing to d:/RCompile/CRANguest/R-release/lib/00LOCK-partitions/00new/partitions/libs/i386

*** arch - x64
d:/Compiler/rtools40/mingw64/bin/g++ -std=gnu++11  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -c app.cpp -o app.o
d:/Compiler/rtools40/mingw64/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c partitions.c -o partitions.o
In function 'c_sort.part.0',
    inlined from 'c_sort' at partitions.c:263:6:
partitions.c:268:20: warning: argument 1 value '18446744071562067969' exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
   int *a = (int *) calloc(max + 1, sizeof(int));
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from partitions.c:2:
partitions.c: In function 'c_sort':
D:/Compiler/rtools40/mingw64/x86_64-w64-mingw32/include/stdlib.h:455:17: note: in a call to allocation function 'calloc' declared here
   void *__cdecl calloc(size_t _NumOfElements,size_t _SizeOfElements);
                 ^~~~~~
d:/Compiler/rtools40/mingw64/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c partitions_init.c -o partitions_init.o
d:/Compiler/rtools40/mingw64/bin/gcc  -I"D:/RCompile/recent/R-4.0.2/include" -DNDEBUG     -I"d:/Compiler/gcc-4.9.3/local330/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign -c permutations.c -o permutations.o
d:/Compiler/rtools40/mingw64/bin/g++ -std=gnu++11 -shared -s -static-libgcc -o partitions.dll tmp.def app.o partitions.o partitions_init.o permutations.o -Ld:/Compiler/gcc-4.9.3/local330/lib/x64 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R-4.0.2/bin/x64 -lR
installing to d:/RCompile/CRANguest/R-release/lib/00LOCK-partitions/00new/partitions/libs/x64
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
*** arch - i386
*** arch - x64
** testing if installed package can be loaded from final location
*** arch - i386
*** arch - x64
** testing if installed package keeps a record of temporary installation path
* MD5 sums
packaged installation of 'partitions' as partitions_1.10-0.zip
* DONE (partitions)

Best wishes

Robin

pegeler commented 4 years ago

Hello @RobinHankin

Sorry for the run-around. I was surprised to see this warning creep up again. I was able to reproduce it and have found a fix. It appears that the Windows implementation of the compiler is trying to optimize where it shouldn't. Using the volatile qualifier asks the compiler not to. I have eliminated the warning on my test machine.

Please let me know if you find anything else.

RobinHankin commented 4 years ago

hi again, happy to merge...winbuilder found a couple of things to complain about (which were separate issues that I'll have to sort out). Best wishes , Robin

RobinHankin / partitions

Closing #9 and #11; accepting data in non-standard form for durfee and conjugate functions #12

Codecov Report