CellularResolutions.m2 pull request for JSAG (try 2) #3969
Conversation
|
Thanks, @jkyang92 (and @OlaSobieska) ! I will propose to have an M2Internals discussion of how to review a general JSAG-linked PR in various scenarios (new package, existing package, etc.) and possible outcomes (for the PR and for the JSAG submission). |
As I mentioned here, GitHub doesn't allow comments on lines not changed in given PR, nor does it even show beyond a few lines surrounding the diff (e.g. I can see that this package has An alternative is to also leave comments on the previously listed pull requests, but even then it's hard to leave a comment on v1 and check that is hasn't already been changed in v2, etc. |
No, there are still plenty of options for constructive feedback. |
Do you mean just commenting as if this was an issue? Could you give an example of giving feedback using the PR review interface here? |
Please (from now) comment only on the package associated to this PR here. Request: make your comments short and to the point and moderate/neutral in tone. |
|
The code looks quite clean, but as we know every program can be made one line shorter. Perhaps this is the line: |
There was a problem hiding this comment.
(Apart from #3969 (comment))
there is an easy-to-overlook thing: for each net definition there should be texMath definition --- otherwise M2Web and VSCode new interface would not display your new types nicely.
https://github.com/Macaulay2/M2/blob/b1c5e37bf0974f2adebe069ca85b8bd3e6acf13a/M2/Macaulay2/packages/CellularResolutions.m2#L583C1-L597C1
Also, consider documenting both net and texMath (perhaps in the same doc node).
There was a problem hiding this comment.
It is beneficial to use = instead of := for "private" functions, e.g.,
https://github.com/Macaulay2/M2/blob/b1c5e37bf0974f2adebe069ca85b8bd3e6acf13a/M2/Macaulay2/packages/CellularResolutions.m2#L69C1-L83C1
These (with =) would be exposed by debug Package.
There was a problem hiding this comment.
Even if TypicalValue=>CellComplex is specified, consider adding the return type, e.g.,
cellComplex(Ring,List) := CellComplex => {} >> o -> (R,maxCells) -> (
in
cellComplex = method(Options=>true, TypicalValue=>CellComplex)
cellComplex(Ring,List) := {} >> o -> (R,maxCells) -> (
(realMaxCells,allCells) := maxAndAllCells maxCells;
mkCellComplex(R, allCells, realMaxCells)
)
There was a problem hiding this comment.
Several more suggestions:
(1) Consider dropping paretheses: all paretheses can be removed in,for instance,
dim(Cell) := (cell) -> cell.cellDimension
(2) There is a new syntactic sugar that can be used: lines like
zeroDimCells := if p#?true then p#true else {};
can be replaced by
zeroDimCells := p#true ?? {};
(3) This line in isWellDefined, for instance,
if not isSubmodule M then return false;
could be more verbose if debugLevel > 0, for instance,
if not isSubmodule M then (
if debugLevel > 0 then << "not well defined: expected a submodule" << endl;
return false;
)
There was a problem hiding this comment.
One last comment for now is that you may want a short example appear on the main doc page for the package.
(By the way, some of the remarks so far are produced reviewing-in-pairs with @mikestillman )
This is the pull request for the CellularResolutions package for JSAG. The changes are only the author emails since the package was already previously included in the Macualay2 distribution.
See also, these previous pull requests: #2833, #2844
I've created a draft pull request this time so it won't get merged by accident. See #3968
CC: @OlaSobieska