Lisp-Stat / lisp-stat

Lisp-Stat main system
https://lisp-stat.github.io/lisp-stat
Microsoft Public License
145 stars 11 forks source link

ls:fivenum returns Tukey's 5-number summary if :tukey is true #7

Closed diminou closed 3 years ago

diminou commented 3 years ago

fivenum now takes a keyword argument :tukey When :tukey is truthy, fivenum returns a 5-number summary computed with Tukey's hinges. The default behaviour is unchanged - quartiles 0, 1, 2, 3, 4 are returned.

Closes #3

diminou commented 3 years ago

Thanks for your review.

Could you also point me to information on running tests locally? Being new to CL, I did not quite manage to run them nor to find "canonical" recommendations for running tests.

Symbolics commented 3 years ago

Could you also point me to information on running tests locally? Being new to CL, I did not quite manage to run them nor to find "canonical" recommendations for running tests.

The canonical way for running tests is with ASDF, the de-facto build system for Common Lisp. Every implementation I am aware of includes a version.

The form for testing lisp-stat is (asdf:test-system :lisp-stat). That will test the functions defined in lisp-stat. You can replace :lisp-stat with any of the other systems in the Lisp-Stat github repo. Each system is designed to be independently usable/useful, so they have their own tests.

If you are running emacs with slime, you can use the slime shortcuts. With this you can type , in the REPL buffer and then give slime the command test-system. Slightly shorter and more convenient.

Symbolics commented 3 years ago

Being new to CL ...

Since you're new to CL, I'd like to introduce you to a de-structuring bind construct: let+. The pattern you need in the first few lines of the Tukey implementation:

(let* ((mn-md-mx (nu:quantiles x '(0 0.5 1)))
        (mn (aref mn-md-mx 0))
            (md (aref mn-md-mx 1))
            (mx (aref mn-md-mx 2))

is common enough that several extensions have been created to deal with it and patterns like it. Using let+ you can reduce the above to:

(let+ ((#(mn md mx) (nu:quantiles x '(0 0.5 1))) ; this is untested in the pull request comment
         ...

let+ is used throughout lisp-stat systems, but so far hasn't been included in the lisp-stat system itself. So, if you choose to implement this, you'll need to add it as a dependency in the system definition file (.asdf file). Let+ is generally useful enough that it's fine to add it for all lisp-stat systems.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

diminou commented 3 years ago

Thanks!

It turns out there were a couple of naming conflicts between parachute and data-frame that broke tests on my machine. Please let me know if this should be included in a separate PR or done differently.

Symbolics commented 3 years ago

It turns out there were a couple of naming conflicts between parachute and data-frame that broke tests on my machine. Please let me know if this should be included in a separate PR or done differently.

Oh, what were they? I hadn't encountered them, but I mostly work on the individual packages and may have missed them. data-frame usescl-unit2`, which I suspect might be where the conflicts are. Someday I'd like to convert everything to parachute, but that's lower priority at the moment since everything works.

Probably best to open an issue describing the conflicts and we can discuss a fix there, since it's a separate issue.

diminou commented 3 years ago

The conflicts were:

; compiling (IN-PACKAGE :CL-USER)
; compiling (UIOP/PACKAGE:DEFINE-PACKAGE :LS-TESTS ...)
debugger invoked on a SIMPLE-ERROR in thread #<THREAD "main thread" RUNNING {1001E06743}>: Can't inherit "NAME" from "PARACHUTE", it is inherited from "DATA-FRAME"

and

; compiling (IN-PACKAGE :CL-USER)
; compiling (UIOP/PACKAGE:DEFINE-PACKAGE :LS-TESTS ...)
debugger invoked on a SIMPLE-ERROR in thread #<THREAD "main thread" RUNNING {1001E06743}>: Can't inherit "SUMMARY" from "PARACHUTE", it is inherited from "DATA-FRAME"

when trying to test the system.

Fixed by adding the lines

  (:shadowing-import-from :parachute #:name)
  (:shadowing-import-from :parachute #:summary)

to tests/tstpkg.lisp.

I will revert the change and add a separate issue + PR if you want.

Symbolics commented 3 years ago

No need, I'll have a look tomorrow. It's late here at the moment.