Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Terms Chapter RC Review #335

Closed
43 tasks done
wesbland opened this issue Nov 17, 2020 · 12 comments
Closed
43 tasks done

Terms Chapter RC Review #335

wesbland opened this issue Nov 17, 2020 · 12 comments
Assignees
@tonyskjellum @RolfRabenseifner @GuillaumeMercier @dholmes-epcc-ed-ac-uk @wgropp @puribangalore @jaegerj @cblaas
Labels
chap-bindings Language Bindings Chapter Committee Chapter Committee Change Changes to be made by the respective chapter committee(s)
Projects
MPI 4.0 Ratification
Milestone
2020-12

Comments

@wesbland
Copy link
Member

wesbland commented Nov 17, 2020 •
edited by cblaas
Loading

Problem

These are comments on the RC from @bgoglin, @abouteiller and @RolfRabenseifner:

From @bgoglin:

Section 2.5.1

however, MPI need not deallocate the object immediately.

needs?

Section 2.5.4

The interline space between constants seems bigger in the first list than in the second list.

Section 2.5.8

Count values need to be large enough to encode any value used for expressing element counts, stride, offset, index, displacement, typemaps in memory, typemaps in file views, etc.

Not sure why counts and typemaps are plural while others are not.

There are some linebreaks that don't seem properly placed, causing the line to look too short:

  • Line 13, between "support" and "int" ?
  • Line 20, between "and" and "INTEGER" ?

Section 2.9.2

line 6:

it is strongly recommended that it not use

it does not use?

Additional comments from @abouteiller:

pdf 50, chap 2.2, pp 10:9

Need to verify if statement “MPI identifiers are limited to 30 characters)” is still valid. We may have spilled over with the large count variants and some session procedure names.

pdf 52, chap2.4.1, pp12:40

Sentence “Additionally, an MPI operation can be collective or noncollective” starts a new paragraph with a major change of topic. The formatting and indentations are not conducive of that shift. The sentence appears to be part of the prior bullet list item.

To a lesser extent, pp12:8 has the same problem.

Comments from @RolfRabenseifner:

Chap2.4.1, pp12:8-9, 40, 45-56

It is already mentioned by @abouteiller, but this problem is on more locations and should be done totally with noindent,
because some of these heading lines are only one line and some are more than one line.

Chap 2.2, pp 9:40

In C and the Fortran mpi_f08 module, all ...

Chap 2.2, pp 9:42

In the Fortran mpi module and mpif.h file, ...

Chap 2.2, pp 9:46-47

If the routine is not associated with a class, the name should be of the form
MPI_Action_subset or MPI_ACTION_SUBSET in C and Fortran.

Chap 2.2, pp 10:9

As already mentioned by @abouteiller, we have a problem with the 30 character procedure name limit
(only routines without MPI_T_...):
. . . . .
(comment was moved to #403)

Todo

  • MERGED (VARIOUS PR's)
  • merged & done --> moved to separate issue: Concerning the removal of the name-length limit see issue Drop MPI identifiers are limited to 30 characters #403 and PR 432 https://github.com/mpi-forum/mpi-standard/pull/432 as well as issue Changes for RC 40 as discussed at the MPI forum Dec 7-10, 2020 for Chapters 1, 2, 8, 16-18, A, B & others #405 and PR 434 https://github.com/mpi-forum/mpi-standard/pull/434
  • merged ??? ??, 2020 --> PR 350: https://github.com/mpi-forum/mpi-standard/pull/350 --> MPI-4.0-RC 21:13/14: formatting ?: bad line break (forced?)
  • merged Dec 11, 2020 --> PR 402: https://github.com/mpi-forum/mpi-standard/pull/402 --> MPI-4.0-RC 23:26,29: substitute MPI-3.2 by MPI-4.0 (needed throughout the standard)
  • merged Dec 18, 2020 --> PR 433: https://github.com/mpi-forum/mpi-standard/pull/433 --> MPI-4.0-RC 11:12-18: MPI function specification twice (C and Fortran with mpi_f08) write: version(s)
  • MERGED Dec 21, 2020 --> PR 437: https://github.com/mpi-forum/mpi-standard/pull/437
  • MPI-4.0-RC 11:18: after this line add: Some MPI procedures have two interfaces for a given language support (see Sections 2.5.6 and 2.5.8).
  • MPI-4.0-RC 12:1: write what it returns consistently at Completion and Freeing, therefore delete at Completion: "to the application"
  • MPI-4.0-RC 12:17: Figure caption - do not capitalize all words, only first.
  • MPI-4.0-RC 12:30: Figure caption - do not capitalize all words, only first.
  • MPI-4.0-RC 13:8: Figure caption - do not capitalize all words, only first.
  • MPI-4.0-RC 13:12: longer dash ?: ... - defined as follows:
  • MPI-4.0-RC 21:6: add a "the": ... the MPI_Count type ...
  • MPI-4.0-RC 21:40: hard to read/understand if not familiar with Fortran, therefore write: ... assumed type (i.e., TYPE(*)) and assumed rank (i.e., DIMENSION(..)), ...
  • MPI-4.0-RC 22:13: add "previously": Some of the previously deprecated ...
  • MPI-4.0-RC 23:30 compare 773:44-47: add: or c_sizeof()
  • MPI-4.0-RC 23:34 compare 773:44-47: adjust footnote 5 to: Fortran intrinsic. storage_size() returns the size in bits instead of bytes, see Section 16.4.
  • MERGED Dec 19, 2020 --> FORMATTING / EDITOR CHANGES --> PR 438: https://github.com/mpi-forum/mpi-standard/pull/438
  • MPI-4.0-RC 12:8-9: move left "MPI operations are available..." (see also reviewers above)
  • MPI-4.0-RC 12:40: move left "Additionally, an MPI operation can be..." (see also reviewers above)
  • MPI-4.0-RC 16:19: too much white space (vertical)
  • MPI-4.0-RC 18/19: bad page break (forced?)
  • MPI-4.0-RC 19:20: move left "The constants..." (seems okay, but hard to read that way)
  • MPI-4.0-RC 20:27: no blank (~) in INTEGER (KIND=...
  • MPI-4.0-RC 21:5: twice: no blank (~) in INTEGER (KIND=...
  • MPI-4.0-RC 21:16: no blank (~) in INTEGER (KIND=...
  • MPI-4.0-RC 21:21: no blank (~) in INTEGER (KIND=...
  • MPI-4.0-RC 22:33-34: different macros/sizes for: COMM_COPY_ATTR_FUNCTION and MPI_NULL_COPY_FN --> see (last) comment of Bill below
  • MPI-4.0-RC 23:18,20: twice wrong macro, that caused function instead of Callback Index!
  • MPI-4.0-RC 23:37: Table caption: why is Removed capitalized?
  • MERGED Dec 21, 2020 --> REVIEWER COMMENTS --> PR 440: https://github.com/mpi-forum/mpi-standard/pull/440
  • Section 2.5.8: Count values need to be large enough to encode any value used for expressing element counts, stride, offset, index, displacement, typemaps in memory, typemaps in file views, etc. --> counts and typemaps are plural while others are not
  • Chap 2.2, pp 9:40: In C and the Fortran mpi_f08 module, all ...
  • Chap 2.2, pp 9:42: In the Fortran mpi module and mpif.h file, ...
  • Chap 2.2, pp 9:46-47: Rolf's suggestion: "If the routine is not associated with a class, the name should be of the form MPI_Action_subset or MPI_ACTION_SUBSET in C and Fortran."
  • ----------------------- NOT DONE --> REJECTED AS UNNEEDED OR INCORRECT -----------------------
  • REVIEWER COMMENTS --> REJECTED AS UNNEEDED OR INCORRECT
  • Section 2.5.1: however, MPI need not deallocate the object immediately. needs? --> see comments of Dan and Rolf below --> REJECTED see comment of Bill below
  • Section 2.9.2 27:6: that it not use --> that it does not use --> REJECTED see comment of Bill below
  • FORMATTING / EDITOR CHANGES --> REJECTED AS UNNEEDED OR INCORRECT
  • MPI-4.0-RC 15:30: bad line break, seems to fit in line --> no, it does not fit --> REJECTED see comment of Bill below
  • MPI-4.0-RC 12:45-46: -not sure about this one- move left "Collective MPI operations..." (see also reviewer/Rolf above) --> REJECTED see comment of Bill below
  • MPI-4.0-RC 22:8 & 39: do we really want to write: INTEGERs --> REJECTED see comment of Bill below

References

All changes and their formatting are checked and verified in
https://github.com/mpi-forum/mpi-issues/files/5725671/mpi40-report_Issue405_PR439.pdf
@wesbland wesbland added mpi-4.0 chap-bindings Language Bindings Chapter Committee Chapter Committee Change Changes to be made by the respective chapter committee(s) labels Nov 17, 2020
@wesbland wesbland added this to the 2020-12 milestone Nov 17, 2020
@wesbland wesbland added this to Triage in MPI 4.0 Ratification via automation Nov 17, 2020
@wesbland wesbland changed the title Terms RC Review Terms Chapter RC Review Nov 17, 2020
@wesbland wesbland moved this from Triage to To Do in MPI 4.0 Ratification Nov 18, 2020
@dholmes-epcc-ed-ac-uk
Copy link
Member

need[s?] not -> is not required to
it [does?] not use -> it avoid using

@RolfRabenseifner
Copy link

As far as I understood from discussions in the web, "MPI need not deallocate ..." is correct because "need" is used as a modal verb and therefore (at least in Amercan English) does not get the "s" and does not get the "to".

"it [does?] not use" seems to really require the "does".

In this late state, I would always prefere to only correct the grammer and not to substitute the wording.

@wesbland
Copy link
Member Author

Added some additional comments from @abouteiller

@RolfRabenseifner
Copy link

A list of all MPI-4.0 procedures in the Function index and #characters of each function name, see
rc-40-procedure-names_sorted.txt

This was referenced Nov 30, 2020
Closed
Closed
@wgropp
Copy link

wgropp commented Dec 1, 2020

Good catch on 2.5.4 - the wrong macros were used for the second list. That one is fixed in PR https://github.com/mpi-forum/mpi-standard/pull/345

@wgropp
Copy link

wgropp commented Dec 1, 2020

Of these in Section 2.5.8,

Line 13, between "support" and "int" ?
Line 20, between "and" and "INTEGER" ?

Line 13 is fixed in PR 350 through the addition of a \ctypeshort macro. Line 20 is awkward, but is intentional - the intent was to keep the "INTEGER (KIND=xxx)" together, and that led to the bad break.

@RolfRabenseifner
Copy link

RolfRabenseifner commented Dec 7, 2020 •
edited by cblaas
Loading

The names problem is even harder: In some new constants and callback prototypes and one existing MPI-3.1 callback prototype, the 25 year old limit of 30 characters for any MPI names is violated:
. . . . .
(comment was moved to #403)

@cblaas cblaas mentioned this issue Dec 10, 2020
Closed
4 tasks
@dholmes-epcc-ed-ac-uk
Copy link
Member

Is this issue now fully addressed or are there outstanding points to be dealt with later?

@dholmes-epcc-ed-ac-uk dholmes-epcc-ed-ac-uk moved this from To Do for MPI 4.0 to In Progress in MPI 4.0 Ratification Dec 16, 2020
@wgropp
Copy link

wgropp commented Dec 19, 2020

15:30 above: This break is because the next token does not fit. This is working as intended. To remove/reduce the break, a re-write of the paragraph would be required.

@wgropp
Copy link

wgropp commented Dec 21, 2020

I Handled the Reviewer comments in PR 440. The two not checked I rejected as unneeded or incorrect (is there any way to not that on the checkboxes?)

@cblaas
Copy link

cblaas commented Dec 21, 2020

@wgropp Thanks a lot, seems we are (almost) done with this!

I rearranged the checkboxes to mark some points raised as REJECTED.

Concerning editor changes not done and therefore still unchecked, I assume these are also rejected (twice & fine with me) or you forgot to check (once; last point). If you agree with my evaluation please check the 3 checkboxes.

@wgropp
Copy link

wgropp commented Dec 21, 2020

Yes, the unchecked ones were rejected. That last one was corrected by changing the macro; I'm not sure that the sizes are the same, but the macros selected are now correct.

@cblaas cblaas closed this as completed Dec 21, 2020
MPI 4.0 Ratification automation moved this from In Progress to Done Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tonyskjellum tonyskjellum

@RolfRabenseifner RolfRabenseifner

@GuillaumeMercier GuillaumeMercier

@dholmes-epcc-ed-ac-uk dholmes-epcc-ed-ac-uk

@wgropp wgropp

@puribangalore puribangalore

@jaegerj jaegerj

@cblaas cblaas

Labels
chap-bindings Language Bindings Chapter Committee Chapter Committee Change Changes to be made by the respective chapter committee(s)
Projects
No open projects
MPI 4.0 Ratification
Done
Milestone
2020-12
Development

No branches or pull requests
9 participants
@tonyskjellum @wesbland @RolfRabenseifner @GuillaumeMercier @dholmes-epcc-ed-ac-uk @wgropp @puribangalore @jaegerj @cblaas